diff --git a/src/branch.rs b/src/branch.rs new file mode 100644 index 000000000..18ef2e8b8 --- /dev/null +++ b/src/branch.rs @@ -0,0 +1,150 @@ +//! Issue branch selection: start a fresh Issue branch, or continue the one +//! already on origin (ADR-0002). + +use anyhow::{Context, Result, bail}; + +use crate::git::Git; +use crate::github::{self, PrState, PullRequest}; +use crate::issue::IssueUrl; + +pub enum Selection { + /// No Issue branch has been used yet: start `branch` from the Base branch. + Fresh { branch: String }, + /// `branch` is on origin, with no PR or the open `pr`. + Continuation { + branch: String, + pr: Option, + }, +} + +impl Selection { + pub fn branch(&self) -> &str { + match self { + Selection::Fresh { branch } | Selection::Continuation { branch, .. } => branch, + } + } + + /// The Base branch: the open PR's base in a Continuation that has one, + /// otherwise `checked_out`, the branch checked out in the launch + /// directory (`None` on a detached HEAD). Says so on stderr when the PR's + /// base replaces a different checked-out branch. + pub fn base_branch(&self, checked_out: Option<&str>) -> Result { + if let Selection::Continuation { + branch, + pr: Some(pr), + } = self + { + if let Some(checked_out) = checked_out.filter(|&c| c != pr.base) { + eprintln!( + "thirdshift: continuing {branch} and its PR {}, so the Base branch is {}, not the checked-out {checked_out}", + pr.url, pr.base + ); + } + return Ok(pr.base.clone()); + } + checked_out + .map(String::from) + .context("HEAD is detached; check out the branch the work should be based on") + } +} + +/// Pick the Issue branch for `issue` from the Issue branches on origin (one +/// `git ls-remote`) and their PRs in any state (one `gh pr list`). Fails if a +/// local copy of the chosen branch in the launch repository differs from +/// origin's: the Run replaces it and deletes it at cleanup. +pub fn select(launch: &Git, issue: &IssueUrl) -> Result { + let first_branch = branch_name(issue, 1); + let remote = launch.run(&[ + "ls-remote", + "--heads", + "origin", + &format!("refs/heads/{first_branch}"), + &format!("refs/heads/{first_branch}-branch-*"), + ])?; + let on_origin: Vec<(u64, String)> = remote + .lines() + .filter_map(|line| { + let (sha, name) = line.split_once('\t')?; + let number = branch_number(issue, name.strip_prefix("refs/heads/")?)?; + Some((number, sha.to_string())) + }) + .collect(); + let prs: Vec<(u64, PullRequest)> = + github::pull_requests_with_head_prefix(issue, &first_branch)? + .into_iter() + .filter_map(|pr| Some((branch_number(issue, &pr.head)?, pr))) + .collect(); + + let highest = on_origin + .iter() + .map(|(number, _)| *number) + .chain(prs.iter().map(|(number, _)| *number)) + .max(); + let Some(highest) = highest else { + check_local_branch(launch, &first_branch, None)?; + return Ok(Selection::Fresh { + branch: first_branch, + }); + }; + let branch = branch_name(issue, highest); + // A branch can have had several PRs; the newest one decides. + let pr = prs + .into_iter() + .filter_map(|(number, pr)| (number == highest).then_some(pr)) + .max_by_key(|pr| pr.number); + let origin_sha = on_origin + .into_iter() + .find_map(|(number, sha)| (number == highest).then_some(sha)); + match (origin_sha, pr) { + (Some(origin_sha), pr) if pr.as_ref().is_none_or(|pr| pr.state == PrState::Open) => { + check_local_branch(launch, &branch, Some(&origin_sha))?; + Ok(Selection::Continuation { branch, pr }) + } + (_, pr) => bail!( + "{branch} can't be continued ({}), and numbered Issue branches are not supported yet", + match pr { + Some(pr) => format!("its PR {} is {}", pr.url, pr.state), + None => "it is gone from origin".to_string(), + } + ), + } +} + +/// A local `branch` in the launch repository must be at `origin_sha`, or not +/// exist when origin has no copy, so no local-only commits are destroyed. +fn check_local_branch(launch: &Git, branch: &str, origin_sha: Option<&str>) -> Result<()> { + let Ok(local_sha) = launch.run(&[ + "rev-parse", + "--verify", + "--quiet", + &format!("refs/heads/{branch}"), + ]) else { + return Ok(()); + }; + match origin_sha { + Some(origin_sha) if origin_sha == local_sha => Ok(()), + Some(_) => bail!( + "the local branch {branch} differs from origin/{branch}; push, reset or delete it first" + ), + None => { + bail!("the local branch {branch} is not on origin; push, rename or delete it first") + } + } +} + +/// `issue-` is branch 1, `issue--branch-` is branch k for k ≥ 2. +fn branch_number(issue: &IssueUrl, branch: &str) -> Option { + let rest = branch.strip_prefix(&branch_name(issue, 1))?; + if rest.is_empty() { + return Some(1); + } + let k: u64 = rest.strip_prefix("-branch-")?.parse().ok()?; + (k >= 2 && branch == branch_name(issue, k)).then_some(k) +} + +fn branch_name(issue: &IssueUrl, number: u64) -> String { + match number { + 1 => format!("issue-{}", issue.number), + k => format!("issue-{}-branch-{k}", issue.number), + } +} diff --git a/src/github.rs b/src/github.rs index 0d944f9c4..39bef52ea 100644 --- a/src/github.rs +++ b/src/github.rs @@ -1,5 +1,6 @@ //! Asking GitHub, through `gh`, about issues and pull requests. +use std::fmt; use std::process::Command; use anyhow::{Context, Result, bail}; @@ -17,46 +18,111 @@ pub fn issue_is_open(issue: &IssueUrl) -> Result { &issue.repo_slug(), "--json", "state", - ])? - .context("issue not found")?; - Ok(string_field(&json, "state")? == "OPEN") + ])?; + Ok(json["state"].as_str().context("gh output has no state")? == "OPEN") +} + +const PR_FIELDS: &str = "number,url,state,headRefName,baseRefName,isDraft"; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PrState { + Open, + Closed, + Merged, +} + +impl fmt::Display for PrState { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(match self { + PrState::Open => "open", + PrState::Closed => "closed", + PrState::Merged => "merged", + }) + } } pub struct PullRequest { + pub number: u64, pub url: String, - pub state: String, + pub state: PrState, + pub head: String, pub base: String, pub is_draft: bool, } impl PullRequest { pub fn is_open(&self) -> bool { - self.state == "OPEN" + self.state == PrState::Open + } + + fn from_json(json: &Value) -> Result { + let field = |name: &str| -> Result { + json[name] + .as_str() + .map(String::from) + .with_context(|| format!("gh output has no {name}")) + }; + let state = match field("state")?.as_str() { + "OPEN" => PrState::Open, + "CLOSED" => PrState::Closed, + "MERGED" => PrState::Merged, + other => bail!("gh output has an unknown PR state {other}"), + }; + Ok(PullRequest { + number: json["number"].as_u64().context("gh output has no number")?, + url: field("url")?, + state, + head: field("headRefName")?, + base: field("baseRefName")?, + is_draft: json["isDraft"] + .as_bool() + .context("gh output has no isDraft")?, + }) } } /// The pull request whose head is `branch`, if `gh` finds one. pub fn pull_request_for(issue: &IssueUrl, branch: &str) -> Result> { - let json = gh_json(&[ + let json = match gh_json(&[ "pr", "view", branch, "--repo", &issue.repo_slug(), "--json", - "url,state,baseRefName,isDraft", - ])?; - let Some(json) = json else { - return Ok(None); + PR_FIELDS, + ]) { + Ok(json) => json, + Err(error) if format!("{error:#}").contains("no pull requests found") => return Ok(None), + Err(error) => return Err(error), }; - Ok(Some(PullRequest { - url: string_field(&json, "url")?, - state: string_field(&json, "state")?, - base: string_field(&json, "baseRefName")?, - is_draft: json["isDraft"] - .as_bool() - .context("gh output has no isDraft")?, - })) + PullRequest::from_json(&json).map(Some) +} + +/// Every pull request in this repository, in any state, whose head branch +/// starts with `prefix`. PRs from forks are left out: their heads are not +/// this repository's branches. +pub fn pull_requests_with_head_prefix(issue: &IssueUrl, prefix: &str) -> Result> { + let json = gh_json(&[ + "pr", + "list", + "--repo", + &issue.repo_slug(), + "--state", + "all", + "--search", + &format!("head:{prefix}"), + "--limit", + "1000", + "--json", + &format!("{PR_FIELDS},isCrossRepository"), + ])?; + json.as_array() + .context("gh pr list did not return a list")? + .iter() + .filter(|pr| pr["isCrossRepository"] != Value::Bool(true)) + .map(PullRequest::from_json) + .collect() } /// Mark the pull request whose head is `branch` ready for review. @@ -92,29 +158,20 @@ fn gh(args: &[&str]) -> Result<()> { Ok(()) } -/// Run `gh ` and parse its JSON output, or `None` if `gh` found no -/// pull request. -fn gh_json(args: &[&str]) -> Result> { +/// Run `gh ` and parse its stdout as JSON, failing with gh's stderr if +/// it exits non-zero. +fn gh_json(args: &[&str]) -> Result { + let command = format!("gh {}", args[..2].join(" ")); let output = Command::new("gh") .args(args) .output() .context("could not run gh")?; - let command = format!("gh {}", args.join(" ")); if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - if stderr.contains("no pull requests found") { - return Ok(None); - } - bail!("{command} failed: {}", stderr.trim()); + bail!( + "{command} failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); } serde_json::from_slice(&output.stdout) - .map(Some) .with_context(|| format!("{command} returned invalid JSON")) } - -fn string_field(json: &Value, name: &str) -> Result { - json[name] - .as_str() - .map(String::from) - .with_context(|| format!("gh output has no {name}")) -} diff --git a/src/main.rs b/src/main.rs index 1ef497d74..9edd196b6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,3 +1,4 @@ +mod branch; mod failed_run; mod git; mod github; diff --git a/src/preflight.rs b/src/preflight.rs index 44178d250..e9587c455 100644 --- a/src/preflight.rs +++ b/src/preflight.rs @@ -7,9 +7,10 @@ use crate::git::Git; use crate::github; use crate::issue::IssueUrl; -/// Check that a Run on `issue` from the `launch` repository makes sense, and -/// return its Base branch. -pub fn check(launch: &Git, issue: &IssueUrl) -> Result { +/// Check that a Run on `issue` from the `launch` repository makes sense. The +/// Base branch is checked separately, by [`check_base_branch`], once Issue +/// branch selection has picked it. +pub fn check(launch: &Git, issue: &IssueUrl) -> Result<()> { let origin = launch.run(&["config", "remote.origin.url"])?; if !issue.matches_origin(&origin) { bail!( @@ -25,16 +26,12 @@ pub fn check(launch: &Git, issue: &IssueUrl) -> Result { bail!("git {key} is not set; the agent needs it to commit"); } } - base_branch(launch) + Ok(()) } -/// The checked-out branch, provided it exists on origin and has no commits -/// origin lacks. -fn base_branch(launch: &Git) -> Result { - let base = launch.run(&["branch", "--show-current"])?; - if base.is_empty() { - bail!("HEAD is detached; check out the branch the work should be based on"); - } +/// Check that the Base branch `base` exists on origin and that the local +/// `base`, if any, has no commits origin lacks. +pub fn check_base_branch(launch: &Git, base: &str) -> Result<()> { let on_origin = launch.run(&[ "ls-remote", "--heads", @@ -44,10 +41,17 @@ fn base_branch(launch: &Git) -> Result { if on_origin.is_empty() { bail!("base branch {base} does not exist on origin; push it first"); } - launch.run(&["fetch", "origin", &base])?; + launch.run(&["fetch", "origin", base])?; + let local = format!("refs/heads/{base}"); + if launch + .run(&["rev-parse", "--verify", "--quiet", &local]) + .is_err() + { + return Ok(()); + } let ahead = launch.run(&["rev-list", "--count", &format!("origin/{base}..{base}")])?; if ahead != "0" { bail!("local {base} is {ahead} commit(s) ahead of origin/{base}; push them first"); } - Ok(base) + Ok(()) } diff --git a/src/prompt.rs b/src/prompt.rs index a7dd5c8d3..cf10e7993 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -16,6 +16,38 @@ pub fn fresh(issue: &IssueUrl, base: &str, branch: &str) -> String { ) } +/// The continuation prompt, for a run that picks up an existing Issue branch. +/// With `pr_url`, the agent updates that PR; otherwise it creates one. +pub fn continuation(issue: &IssueUrl, base: &str, branch: &str, pr_url: Option<&str>) -> String { + let pr = match pr_url { + Some(url) => format!( + "Update PR {url} using /thirdshift:pr, rewriting its body to cover the whole branch, marked ready for review." + ), + None => format!( + "Create a pull request against {base} using /thirdshift:pr, marked ready for review." + ), + }; + format!( + "/thirdshift:implement {url}\n\ + \n\ + You are continuing work on branch {branch}, which already has commits (see git log {base}..HEAD). Build on them; don't start over.\n\ + \n\ + The base branch is {base}. Review with /thirdshift:code-review using {base} as the fixed point.\n\ + \n\ + Address the Standards and Spec findings you agree with.\n\ + \n\ + Push branch {branch}.\n\ + \n\ + {pr}\n\ + \n\ + In the PR body, add an \"Unaddressed findings\" section listing each skipped finding under Standards or Spec, with at least a one-line reason.\n\ + \n\ + Include \"Closes #{number}\" in the PR body.\n", + url = issue.url, + number = issue.number, + ) +} + /// The conflict Repair prompt, for a merge of the Base branch left in progress /// with conflicts. pub fn conflict_repair(issue: &IssueUrl, base: &str, branch: &str, pr_url: &str) -> String { diff --git a/src/run.rs b/src/run.rs index ee48af3a2..7ae132487 100644 --- a/src/run.rs +++ b/src/run.rs @@ -4,6 +4,7 @@ use std::path::PathBuf; use anyhow::{Context, Result, anyhow, bail}; +use crate::branch::{self, Selection}; use crate::failed_run::{self, FailedRun}; use crate::git::Git; use crate::github; @@ -23,24 +24,41 @@ pub fn run(issue: &IssueUrl) -> Result { let timestamp = chrono::Utc::now().format("%Y%m%dT%H%M%SZ").to_string(); let launch = Git::new(std::env::current_dir().context("no current directory")?); - let base = preflight::check(&launch, issue)?; - let branch = format!("issue-{}", issue.number); + preflight::check(&launch, issue)?; + let checked_out = launch + .run(&["symbolic-ref", "--quiet", "--short", "HEAD"]) + .ok(); + let selection = branch::select(&launch, issue)?; + let branch = selection.branch().to_string(); + let base = selection.base_branch(checked_out.as_deref())?; + preflight::check_base_branch(&launch, &base)?; if interrupt::requested() { return Err(anyhow!("interrupted").into()); } - let worktree = Worktree::create_fresh(&launch, &issue.repo, &branch, &base)?; + let (worktree, prompt) = match &selection { + Selection::Fresh { .. } => ( + Worktree::create_fresh(&launch, &issue.repo, &branch, &base)?, + prompt::fresh(issue, &base, &branch), + ), + Selection::Continuation { pr, .. } => ( + Worktree::continue_existing(&launch, &issue.repo, &branch, &base)?, + prompt::continuation(issue, &base, &branch, pr.as_ref().map(|pr| pr.url.as_str())), + ), + }; let mut log = session::log_path(issue, ×tamp, "implement")?; - implement(issue, &worktree, &base, ×tamp, &mut log) + implement(issue, &worktree, &base, &prompt, ×tamp, &mut log) .map_err(|error| failed_run::fail(issue, &worktree, &base, &log, error)) } -/// The implement session, the checks on the PR it opened, and keeping that PR -/// mergeable. `log` is left at the most recent session's log. +/// The implement session given `prompt`, the checks on the PR it opened or +/// updated, and keeping that PR mergeable. `log` is left at the most recent +/// session's log. fn implement( issue: &IssueUrl, worktree: &Worktree, base: &str, + prompt: &str, timestamp: &str, log: &mut PathBuf, ) -> Result { @@ -54,13 +72,13 @@ fn implement( session::run(kind, worktree.path(), plugin.path(), prompt, log) }; - run_session("implement", &prompt::fresh(issue, base, branch))?; + run_session("implement", prompt)?; 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.to_lowercase()); + bail!("PR {} is {}, not open", pr.url, pr.state); } if pr.base != base { bail!("PR targets {}, not {base}", pr.base); diff --git a/src/worktree.rs b/src/worktree.rs index e08b13ff4..ea222ceba 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -27,6 +27,38 @@ impl Worktree { /// Create `branch` fresh from `origin/` in a new worktree next to the /// launch repository's root, named `-`. pub fn create_fresh(launch: &Git, repo: &str, branch: &str, base: &str) -> Result { + launch.run(&["fetch", "origin", base])?; + Self::add( + launch, + repo, + branch, + &["-b", branch], + &format!("origin/{base}"), + ) + } + + /// Check out the existing `branch` from origin in a new worktree next to + /// the launch repository's root, named `-`. `origin/` + /// is fetched too, for the review fixed point. A local `branch`, if any, is + /// reset to origin's: `branch::select` has checked they already match. + pub fn continue_existing(launch: &Git, repo: &str, branch: &str, base: &str) -> Result { + launch.run(&["fetch", "origin", base, branch])?; + Self::add( + launch, + repo, + branch, + &["-B", branch], + &format!("origin/{branch}"), + ) + } + + fn add( + launch: &Git, + repo: &str, + branch: &str, + branch_args: &[&str], + start: &str, + ) -> Result { let root = PathBuf::from(launch.run(&["rev-parse", "--show-toplevel"])?); let path = root .parent() @@ -35,18 +67,13 @@ impl Worktree { let path_arg = path.to_str().context("worktree path is not UTF-8")?; progress::step(format_args!( - "creating worktree {} on {branch} from origin/{base}", + "creating worktree {} on {branch} from {start}", path.display() )); - launch.run(&["fetch", "origin", base])?; - launch.run(&[ - "worktree", - "add", - "-b", - branch, - path_arg, - &format!("origin/{base}"), - ])?; + let mut args = vec!["worktree", "add"]; + args.extend_from_slice(branch_args); + args.extend([path_arg, start]); + launch.run(&args)?; Ok(Worktree { launch: Git::new(root), branch: branch.to_string(), diff --git a/tests/continuation.rs b/tests/continuation.rs new file mode 100644 index 000000000..a82dd5529 --- /dev/null +++ b/tests/continuation.rs @@ -0,0 +1,318 @@ +//! Continuation (ADR-0002): an Issue branch already on origin, with no PR or an +//! open one, is picked up where it stopped instead of started over. + +mod support; + +use support::Scenario; + +/// The agent commits on top of whatever the branch already has and leaves +/// the PR alone: it already exists, or the test doesn't need one. +const AGENT_COMMITS: &str = r#" +echo "more" > more.txt +git add more.txt +git commit -q -m "Continue the work" +"#; + +/// The agent commits and opens a PR for `issue-7` against `main`. +const AGENT_COMMITS_AND_OPENS_PR: &str = r#" +echo "more" > more.txt +git add more.txt +git commit -q -m "Continue the work" +gh pr create --base main --head issue-7 --title "Add feature" --body "Closes #7" +"#; + +#[test] +fn an_issue_branch_with_no_pr_is_continued() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + scenario.agent_does(AGENT_COMMITS_AND_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_eq!(scenario.claude_calls()[0]["branch"], "issue-7"); + assert_eq!( + scenario.origin_log("issue-7"), + Some(vec![ + "Continue the work".to_string(), + "Earlier work".to_string(), + "Initial commit".to_string(), + ]) + ); +} + +#[test] +fn without_a_pr_the_continuation_prompt_asks_for_one() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!( + scenario.claude_calls()[0]["prompt"], + "/thirdshift:implement https://github.com/acme/widgets/issues/7\n\ + \n\ + You are continuing work on branch issue-7, which already has commits (see git log main..HEAD). Build on them; don't start over.\n\ + \n\ + The base branch is main. Review with /thirdshift:code-review using main as the fixed point.\n\ + \n\ + Address the Standards and Spec findings you agree with.\n\ + \n\ + Push branch issue-7.\n\ + \n\ + Create a pull request against main using /thirdshift:pr, marked ready for review.\n\ + \n\ + In the PR body, add an \"Unaddressed findings\" section listing each skipped finding under Standards or Spec, with at least a one-line reason.\n\ + \n\ + Include \"Closes #7\" in the PR body.\n" + ); +} + +#[test] +fn an_issue_branch_with_an_open_pr_is_continued_and_that_pr_is_the_result() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + let pr = scenario.github_has_pr("issue-7", "main", "OPEN"); + scenario.agent_does(AGENT_COMMITS); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{pr}\n")); + assert_eq!(scenario.gh_state()["prs"].as_array().unwrap().len(), 1); + assert_eq!( + scenario.origin_log("issue-7"), + Some(vec![ + "Continue the work".to_string(), + "Earlier work".to_string(), + "Initial commit".to_string(), + ]) + ); +} + +#[test] +fn with_an_open_pr_the_continuation_prompt_asks_to_update_it() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + scenario.github_has_pr("issue-7", "main", "OPEN"); + scenario.agent_does(AGENT_COMMITS); + + scenario.run(&[&scenario.issue_url(7)]); + + let prompt = scenario.first_prompt(); + assert!( + prompt.contains( + "\n\nPush branch issue-7.\n\ + \n\ + Update PR https://github.com/acme/widgets/pull/1 using /thirdshift:pr, rewriting its body to cover the whole branch, marked ready for review.\n\ + \n" + ), + "prompt: {prompt}" + ); + assert!( + !prompt.contains("Create a pull request"), + "prompt: {prompt}" + ); +} + +#[test] +fn the_open_prs_base_is_the_base_branch_whatever_is_checked_out() { + let scenario = Scenario::new(); + scenario.origin_has_branch("develop", "main", &["Develop work"]); + scenario.launch_checks_out("develop"); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + let pr = scenario.github_has_pr("issue-7", "main", "OPEN"); + scenario.agent_does(AGENT_COMMITS); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{pr}\n")); + let prompt = scenario.first_prompt(); + assert!( + prompt.contains("(see git log main..HEAD)") + && prompt.contains("The base branch is main. Review with /thirdshift:code-review using main as the fixed point.") + && !prompt.contains("develop"), + "prompt: {prompt}" + ); + assert!( + result + .stderr + .contains("the Base branch is main, not the checked-out develop"), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn without_an_open_pr_the_checked_out_branch_stays_the_base_branch() { + let scenario = Scenario::new(); + scenario.origin_has_branch("develop", "main", &["Develop work"]); + scenario.launch_checks_out("develop"); + scenario.origin_has_branch("issue-7", "develop", &["Earlier work"]); + scenario.agent_does( + r#" +git commit -q --allow-empty -m "Continue the work" +gh pr create --base develop --head issue-7 --title "Add feature" --body "Closes #7" +"#, + ); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let prompt = scenario.first_prompt(); + assert!( + prompt.contains("Create a pull request against develop"), + "prompt: {prompt}" + ); + assert!( + !result.stderr.contains("Base branch"), + "stderr: {}", + result.stderr + ); +} + +/// Nothing was created: no session, worktree, temp directory or log, and the +/// local Issue branch is still there. +fn assert_nothing_created(scenario: &Scenario, local_branch: &str) { + assert!(scenario.claude_calls().is_empty(), "claude was run"); + assert_eq!(scenario.entries("work"), vec!["widgets"]); + assert_eq!(scenario.entries("tmp"), Vec::::new()); + assert!( + !scenario.path("home/.thirdshift").exists(), + "a log was created" + ); + assert_ne!(scenario.launch_git(&["branch", "--list", local_branch]), ""); +} + +#[test] +fn a_local_issue_branch_that_differs_from_origin_stops_the_run() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + scenario.launch_checks_out("issue-7"); + scenario.launch_git(&["commit", "-q", "--allow-empty", "-m", "Local only"]); + scenario.launch_checks_out("main"); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert_eq!(result.stdout, ""); + assert!( + result + .stderr + .contains("issue-7 differs from origin/issue-7"), + "stderr: {}", + result.stderr + ); + assert_nothing_created(&scenario, "issue-7"); + assert_eq!( + scenario.launch_git(&["log", "-1", "--format=%s", "issue-7"]), + "Local only\n" + ); + assert_eq!(scenario.origin_log("issue-7").map(|log| log.len()), Some(2)); +} + +#[test] +fn a_local_issue_branch_missing_from_origin_stops_the_run() { + let scenario = Scenario::new(); + scenario.launch_git(&["branch", "issue-7"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert!( + result.stderr.contains("issue-7 is not on origin"), + "stderr: {}", + result.stderr + ); + assert_nothing_created(&scenario, "issue-7"); + assert_eq!(scenario.origin_log("issue-7"), None); +} + +#[test] +fn a_local_issue_branch_matching_origin_is_continued() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + scenario.launch_checks_out("issue-7"); + scenario.launch_checks_out("main"); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.origin_log("issue-7").map(|log| log.len()), Some(3)); +} + +#[test] +fn a_failed_runs_branch_is_continued_from_its_failure_commit() { + let scenario = Scenario::new(); + scenario.origin_has_branch( + "issue-7", + "main", + &["Earlier work", "thirdshift: failed run (claude exited 1)"], + ); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!( + scenario.origin_log("issue-7"), + Some(vec![ + "Continue the work".to_string(), + "thirdshift: failed run (claude exited 1)".to_string(), + "Earlier work".to_string(), + "Initial commit".to_string(), + ]) + ); +} + +#[test] +fn selection_asks_github_once_for_the_pr_history() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + scenario.github_has_pr("issue-7", "main", "OPEN"); + scenario.agent_does(AGENT_COMMITS); + + scenario.run(&[&scenario.issue_url(7)]); + + // Pre-flight's `issue view` comes first; everything after it and before + // the post-session `pr view` is selection. The agent here makes no gh + // calls of its own. + let subcommands: Vec = scenario + .gh_calls() + .iter() + .map(|argv| argv[..2].join(" ")) + .collect(); + assert_eq!(subcommands, vec!["issue view", "pr list", "pr view"]); + assert!( + scenario.gh_calls()[1] + .windows(2) + .any(|w| w == ["--state", "all"]), + "gh calls: {:?}", + scenario.gh_calls() + ); +} + +#[test] +fn an_open_pr_supplies_the_base_branch_even_on_a_detached_head() { + let scenario = Scenario::new(); + scenario.launch_git(&["checkout", "-q", "--detach"]); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + let pr = scenario.github_has_pr("issue-7", "main", "OPEN"); + scenario.agent_does(AGENT_COMMITS); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{pr}\n")); + assert!( + scenario.first_prompt().contains("The base branch is main."), + "prompt: {}", + scenario.first_prompt() + ); +} diff --git a/tests/failed_run.rs b/tests/failed_run.rs index 756650402..eb4174fa1 100644 --- a/tests/failed_run.rs +++ b/tests/failed_run.rs @@ -96,7 +96,7 @@ fn no_pr_after_the_session_is_a_failed_run() { #[test] fn a_pr_against_the_wrong_base_is_sent_back_to_draft() { let scenario = Scenario::new(); - scenario.origin_has_branch("develop"); + scenario.origin_has_branch("develop", "main", &[]); scenario.agent_does( "echo feature > feature.txt\ngit add feature.txt\ngit commit -q -m 'Add feature'\n\ gh pr create --base develop --head issue-7 --title 'Add feature' --body 'Closes #7'\n", diff --git a/tests/fakes/gh b/tests/fakes/gh index 95b2aa248..eb9d02279 100644 --- a/tests/fakes/gh +++ b/tests/fakes/gh @@ -7,6 +7,8 @@ State lives in the JSON file named by $FAKE_GH_STATE: "issues": {"": "OPEN" | "CLOSED"}, "prs": [{"number", "url", "head", "base", "state", "isDraft", "title", "body"}]} +Every call's argv is appended to the JSON list in $FAKE_GH_RECORD. + Only the subcommands thirdshift and the fake agents use are supported. Anything else exits 2 so an unexpected call fails loudly. """ @@ -85,15 +87,49 @@ def pr_view(state, positional, flags): if not matches: die(f'no pull requests found for branch "{head}"') pr = matches[-1] - fields = { + wanted = flags["json"].split(",") + print(json.dumps({key: pr_fields(pr)[key] for key in wanted})) + + +def record(args): + path = os.environ["FAKE_GH_RECORD"] + calls = [] + if os.path.exists(path): + with open(path) as f: + calls = json.load(f) + calls.append(args) + with open(path, "w") as f: + json.dump(calls, f, indent=2) + + +def pr_fields(pr): + return { "url": pr["url"], "state": pr["state"], + "headRefName": pr["head"], "baseRefName": pr["base"], "isDraft": pr["isDraft"], "number": pr["number"], + "isCrossRepository": pr.get("isCrossRepository", False), } + + +def pr_list(state, flags): + """Newest first. `--search head:` matches head branches starting + with , as GitHub's search does.""" + wanted_state = flags.get("state", "open").upper() + search = flags.get("search", "") + prefix = search[len("head:"):] if search.startswith("head:") else "" + if search and not prefix: + die(f"fake gh: unsupported search {search}", 2) + prs = [ + pr + for pr in reversed(state["prs"]) + if pr["head"].startswith(prefix) + and (wanted_state == "ALL" or pr["state"] == wanted_state) + ][: int(flags.get("limit", 30))] wanted = flags["json"].split(",") - print(json.dumps({key: fields[key] for key in wanted})) + print(json.dumps([{key: pr_fields(pr)[key] for key in wanted} for pr in prs])) def pr_ready(state, positional, flags): @@ -119,6 +155,7 @@ def issue_view(state, positional, flags): def main(): args = sys.argv[1:] + record(args) state = load() if args[:2] == ["pr", "create"]: positional, flags = parse(args[2:]) @@ -128,6 +165,10 @@ def main(): positional, flags = parse(args[2:]) check_repo(state, flags) pr_view(state, positional, flags) + elif args[:2] == ["pr", "list"]: + positional, flags = parse(args[2:]) + check_repo(state, flags) + pr_list(state, flags) elif args[:2] == ["pr", "ready"]: positional, flags = parse(args[2:]) check_repo(state, flags) diff --git a/tests/fresh_run.rs b/tests/fresh_run.rs index 4c3417dc0..96c5bd274 100644 --- a/tests/fresh_run.rs +++ b/tests/fresh_run.rs @@ -206,3 +206,23 @@ fn marks_a_draft_pr_ready_and_succeeds() { assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], false); } + +#[test] +fn another_issues_branch_and_pr_do_not_count_as_this_issues() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-70", "main", &["Other work"]); + scenario.github_has_pr("issue-70", "main", "MERGED"); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.claude_calls()[0]["branch"], "issue-7"); + assert_eq!( + scenario.origin_log("issue-7"), + Some(vec![ + "Add feature".to_string(), + "Initial commit".to_string() + ]) + ); +} diff --git a/tests/support/mod.rs b/tests/support/mod.rs index fc7e2effb..68713b292 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -15,6 +15,7 @@ //! claude-script.sh what the fake agent does this test //! claude-script.sh. what it does in the n-th session instead, if present //! claude-calls.json what the fake agent was asked to do +//! gh-calls.json every gh command run, by thirdshift or the fake agent //! ``` #![allow(dead_code)] @@ -170,7 +171,8 @@ impl Scenario { .env("GIT_CONFIG_NOSYSTEM", "1") .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_CLAUDE_RECORD", self.path("claude-calls.json")) + .env("FAKE_GH_RECORD", self.path("gh-calls.json")); command } @@ -201,6 +203,61 @@ impl Scenario { } } + /// The prompt of the first `claude` call. + pub fn first_prompt(&self) -> String { + self.claude_calls()[0]["prompt"] + .as_str() + .expect("claude got no prompt") + .to_string() + } + + /// The argv of every `gh` call, in order. + pub fn gh_calls(&self) -> Vec> { + match fs::read_to_string(self.path("gh-calls.json")) { + Ok(text) => serde_json::from_str(&text).unwrap(), + Err(_) => Vec::new(), + } + } + + /// Push `branch` to origin: `from` plus one commit per subject in + /// `commits`, oldest first. + pub fn origin_has_branch(&self, branch: &str, from: &str, commits: &[&str]) { + let seed = self.path("seed"); + git(&self.path(""), &["clone", "-q", &self.github_url(), "seed"]); + git( + &seed, + &["checkout", "-q", "-b", branch, &format!("origin/{from}")], + ); + for (i, subject) in commits.iter().enumerate() { + fs::write(seed.join(format!("{branch}-{i}.txt")), subject).unwrap(); + git(&seed, &["add", "."]); + git(&seed, &["commit", "-q", "-m", subject]); + } + git(&seed, &["push", "-q", "origin", branch]); + fs::remove_dir_all(&seed).unwrap(); + } + + /// Add a PR from `head` into `base` in `state` (`OPEN`, `CLOSED` or + /// `MERGED`) to the fake GitHub and return its URL. + pub fn github_has_pr(&self, head: &str, base: &str, state: &str) -> String { + let mut gh = self.gh_state(); + let prs = gh["prs"].as_array_mut().unwrap(); + let number = prs.len() + 1; + let url = format!("https://github.com/{OWNER}/{REPO}/pull/{number}"); + prs.push(json!({ + "number": number, + "url": url, + "head": head, + "base": base, + "state": state, + "isDraft": false, + "title": format!("Work on {head}"), + "body": "", + })); + self.write_gh_state(&gh); + url + } + /// Commit subjects on `branch` in the origin repo, newest first, or `None` /// if the branch doesn't exist there. pub fn origin_log(&self, branch: &str) -> Option> { @@ -220,16 +277,17 @@ impl Scenario { ) } + /// Fetch origin in the launch clone and check out `branch` there. + pub fn launch_checks_out(&self, branch: &str) { + self.launch_git(&["fetch", "-q", "origin"]); + self.launch_git(&["checkout", "-q", branch]); + } + /// Output of a git command in the origin repo, panicking on failure. pub fn origin_git(&self, args: &[&str]) -> String { git(&self.origin_dir(), args) } - /// Create `branch` on origin, pointing at `main`. - pub fn origin_has_branch(&self, branch: &str) { - self.origin_git(&["branch", branch, "main"]); - } - /// Assert the Run left no worktree, local `branch` or temp directory. pub fn assert_cleaned_up(&self, branch: &str) { assert_eq!(self.entries("work"), vec![REPO]);