diff --git a/src/git.rs b/src/git.rs index 3603333a3..f070cb99b 100644 --- a/src/git.rs +++ b/src/git.rs @@ -1,7 +1,7 @@ //! Running `git` in a directory. use std::path::{Path, PathBuf}; -use std::process::Command; +use std::process::{Command, Output}; use anyhow::{Context, Result, bail}; @@ -23,11 +23,7 @@ impl Git { /// Run `git ` and return its trimmed stdout, failing with git's /// stderr if it exits non-zero. pub fn run(&self, args: &[&str]) -> Result { - let output = Command::new("git") - .args(args) - .current_dir(&self.dir) - .output() - .with_context(|| format!("could not run git {}", args.join(" ")))?; + let output = self.output(args)?; if !output.status.success() { bail!( "git {} failed: {}", @@ -37,4 +33,18 @@ impl Git { } Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) } + + /// Run `git ` and report whether it exited zero, for commands whose + /// exit status is the answer. + pub fn succeeds(&self, args: &[&str]) -> Result { + Ok(self.output(args)?.status.success()) + } + + fn output(&self, args: &[&str]) -> Result { + Command::new("git") + .args(args) + .current_dir(&self.dir) + .output() + .with_context(|| format!("could not run git {}", args.join(" "))) + } } diff --git a/src/prompt.rs b/src/prompt.rs index 679ff2e6b..a7dd5c8d3 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -15,3 +15,17 @@ pub fn fresh(issue: &IssueUrl, base: &str, branch: &str) -> String { 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 { + format!( + "/thirdshift:resolving-merge-conflicts\n\ + \n\ + A merge of origin/{base} into {branch} is in progress in this worktree and has conflicts.\n\ + {branch} implements {url}; its pull request is {pr_url}.\n\ + \n\ + Resolve the conflicts, finish the merge, and push {branch}. Do not rebase or force-push.\n", + url = issue.url, + ) +} diff --git a/src/run.rs b/src/run.rs index 37531f946..573743e57 100644 --- a/src/run.rs +++ b/src/run.rs @@ -9,7 +9,7 @@ use crate::plugin::Plugin; use crate::preflight; use crate::prompt; use crate::session; -use crate::worktree::Worktree; +use crate::worktree::{Merge, Worktree}; /// Take `issue` to a PR and return the PR's URL. The worktree, the local /// Issue branch and the plugin directory are gone when this returns. @@ -22,14 +22,15 @@ pub fn run(issue: &IssueUrl) -> Result { let worktree = Worktree::create_fresh(&launch, &issue.repo, &branch, &base)?; let plugin = Plugin::write()?; - let log = session::log_path(issue, ×tamp, "implement")?; - session::run( - worktree.path(), - plugin.path(), - &prompt::fresh(issue, &base, &branch), - &log, - )?; - worktree.git().run(&["push", "origin", &branch])?; + // Every session runs in the worktree with the plugin loaded, logged as + // `kind` under the Run's timestamp. + let run_session = |kind: &str, prompt: &str| -> Result<()> { + let log = session::log_path(issue, ×tamp, kind)?; + session::run(worktree.path(), plugin.path(), prompt, &log) + }; + + run_session("implement", &prompt::fresh(issue, &base, &branch))?; + worktree.push()?; let pr = github::pull_request_for(issue, &branch)?.context("no PR found")?; if pr.state != "OPEN" { @@ -38,5 +39,15 @@ pub fn run(issue: &IssueUrl) -> Result { if pr.base != base { bail!("PR targets {}, not {base}", pr.base); } + + // Keep the PR mergeable: merge the Base branch, never rebase. + if worktree.merge_base_branch(&base)? == Merge::Conflicted { + run_session( + "repair-1", + &prompt::conflict_repair(issue, &base, &branch, &pr.url), + )?; + worktree.ensure_base_branch_merged(&base)?; + } + worktree.push()?; Ok(pr.url) } diff --git a/src/worktree.rs b/src/worktree.rs index fa0d23fec..9d0f4f5f0 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -3,10 +3,19 @@ use std::path::{Path, PathBuf}; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, bail}; use crate::git::Git; +/// How merging the Base branch into the Issue branch went. +#[derive(Debug, PartialEq, Eq)] +pub enum Merge { + /// Merged, or nothing to merge. + Clean, + /// Conflicted; the merge is left in progress for a conflict Repair. + Conflicted, +} + pub struct Worktree { launch: Git, branch: String, @@ -44,8 +53,46 @@ impl Worktree { self.git.dir() } - pub fn git(&self) -> &Git { - &self.git + /// Push the Issue branch to origin (a no-op if it is already there). + pub fn push(&self) -> Result<()> { + self.git.run(&["push", "origin", &self.branch])?; + Ok(()) + } + + /// 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. + pub fn merge_base_branch(&self, base: &str) -> Result { + self.git.run(&["fetch", "origin", base])?; + match self + .git + .run(&["merge", "--no-edit", "--ff", &format!("origin/{base}")]) + { + Ok(_) => Ok(Merge::Clean), + Err(_) if self.merge_in_progress()? => Ok(Merge::Conflicted), + Err(error) => Err(error), + } + } + + /// Fail unless `origin/` is fully merged into the Issue branch, e.g. + /// after a conflict Repair that left the merge unfinished or aborted it. + pub fn ensure_base_branch_merged(&self, base: &str) -> Result<()> { + if self.merge_in_progress()? { + bail!("the merge of origin/{base} is still in progress"); + } + let upstream = format!("origin/{base}"); + if !self + .git + .succeeds(&["merge-base", "--is-ancestor", &upstream, "HEAD"])? + { + bail!("{upstream} is not merged into {}", self.branch); + } + Ok(()) + } + + fn merge_in_progress(&self) -> Result { + self.git + .succeeds(&["rev-parse", "-q", "--verify", "MERGE_HEAD"]) } } diff --git a/tests/fakes/claude b/tests/fakes/claude index 936fb2269..d0645a451 100644 --- a/tests/fakes/claude +++ b/tests/fakes/claude @@ -2,12 +2,14 @@ """Fake `claude` for the thirdshift test harness. Each call appends a record to the JSON list in $FAKE_CLAUDE_RECORD: its argv, -prompt, working directory, the branch checked out there, and a snapshot of the +prompt, working directory, the branch checked out there, whether a merge is in +progress there, and a snapshot of the --plugin-dir contents (the directory is gone by the time a test looks). -It then runs the per-test bash script $FAKE_CLAUDE_SCRIPT in the working -directory, emits a couple of stream-json lines, and exits with the script's -exit code. +It then runs a per-test bash script in the working directory, emits a couple +of stream-json lines, and exits with the script's exit code. The script for the +n-th call (1-based) is $FAKE_CLAUDE_SCRIPT with `.` appended if that file +exists, else $FAKE_CLAUDE_SCRIPT itself. """ import json @@ -32,6 +34,12 @@ def main(): branch = subprocess.run( ["git", "branch", "--show-current"], capture_output=True, text=True ).stdout.strip() + merging = ( + subprocess.run( + ["git", "rev-parse", "-q", "--verify", "MERGE_HEAD"], capture_output=True + ).returncode + == 0 + ) record_path = os.environ["FAKE_CLAUDE_RECORD"] records = [] @@ -44,6 +52,7 @@ def main(): "prompt": argv[-1] if argv else None, "cwd": os.getcwd(), "branch": branch, + "merging": merging, "plugin_dir": plugin_dir, "plugin_files": plugin_snapshot(plugin_dir) if plugin_dir else {}, } @@ -52,7 +61,10 @@ def main(): json.dump(records, f, indent=2) print(json.dumps({"type": "system", "subtype": "init", "cwd": os.getcwd()}), flush=True) - code = subprocess.run(["bash", "-e", os.environ["FAKE_CLAUDE_SCRIPT"]]).returncode + script = os.environ["FAKE_CLAUDE_SCRIPT"] + if os.path.exists(f"{script}.{len(records)}"): + script = f"{script}.{len(records)}" + code = subprocess.run(["bash", "-e", script]).returncode print(json.dumps({"type": "result", "subtype": "success", "is_error": code != 0}), flush=True) sys.exit(code) diff --git a/tests/mergeable.rs b/tests/mergeable.rs new file mode 100644 index 000000000..5854c7c7b --- /dev/null +++ b/tests/mergeable.rs @@ -0,0 +1,201 @@ +//! Keeping the PR mergeable: once the PR is confirmed, thirdshift merges the +//! Base branch into the Issue branch and pushes, handing a conflicting merge to +//! a conflict Repair session first. + +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" +"#; + +/// While the agent works, someone else pushes `file` with `content` to main. +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" +git -C "$other" push -q origin main +rm -rf "$other" +"# + ) +} + +/// The conflict Repair keeps both sides of feature.txt and finishes the merge. +const REPAIR_RESOLVES_CONFLICT: &str = r#" +printf 'feature\nbase feature\n' > feature.txt +git add feature.txt +git commit -q --no-edit +"#; + +/// origin's issue-7 is the agent's work with main merged in on top. +fn assert_merged_main_into_issue_7(scenario: &Scenario) { + assert_eq!( + scenario.origin_git(&["log", "--first-parent", "--format=%s", "issue-7"]), + "Merge remote-tracking branch 'origin/main' into issue-7\n\ + Add feature\n\ + Initial commit\n" + ); + assert_eq!( + scenario.origin_git(&["rev-parse", "issue-7^2"]), + scenario.origin_git(&["rev-parse", "main"]) + ); +} + +#[test] +fn merges_an_advanced_base_branch_without_a_repair() { + let scenario = Scenario::new(); + // The agent pushes issue-7 itself, so the merge can only reach origin as a + // fast-forward of it (origin rejects anything else). + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}git push -q origin issue-7\n{}", + 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(), 1); + assert_merged_main_into_issue_7(&scenario); +} + +#[test] +fn makes_no_merge_commit_when_the_base_branch_has_not_moved() { + 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!(scenario.claude_calls().len(), 1); + assert_eq!( + scenario.origin_log("issue-7"), + Some(vec![ + "Add feature".to_string(), + "Initial commit".to_string() + ]) + ); +} + +#[test] +fn hands_a_conflicting_merge_to_a_conflict_repair_and_pushes_its_resolution() { + let scenario = Scenario::new(); + // The agent pushes issue-7 itself, so the resolution can only reach origin + // as a fast-forward of it (origin rejects anything else). + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}git push -q origin issue-7\n{}", + base_moves_on("feature.txt", "base feature") + )); + scenario.agent_does_in_session(2, REPAIR_RESOLVES_CONFLICT); + + 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); + let repair = &calls[1]; + assert_eq!( + repair["prompt"], + "/thirdshift:resolving-merge-conflicts\n\ + \n\ + A merge of origin/main into issue-7 is in progress in this worktree and has conflicts.\n\ + issue-7 implements https://github.com/acme/widgets/issues/7; its pull request is https://github.com/acme/widgets/pull/1.\n\ + \n\ + Resolve the conflicts, finish the merge, and push issue-7. Do not rebase or force-push.\n" + ); + assert_eq!(repair["merging"], true); + assert_eq!(repair["branch"], "issue-7"); + assert_eq!(repair["cwd"], calls[0]["cwd"]); + let flags = |call: &serde_json::Value| { + let argv = call["argv"].as_array().unwrap().clone(); + argv[..argv.len() - 1].to_vec() + }; + assert_eq!(flags(repair), flags(&calls[0]), "same plugin and flags"); + assert_merged_main_into_issue_7(&scenario); + assert_eq!( + scenario.origin_git(&["show", "issue-7:feature.txt"]), + "feature\nbase feature\n" + ); +} + +#[test] +fn logs_the_conflict_repair_with_the_runs_shared_timestamp() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}{}", + base_moves_on("feature.txt", "base feature") + )); + scenario.agent_does_in_session(2, REPAIR_RESOLVES_CONFLICT); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let logs = scenario.entries("home/.thirdshift/logs"); + assert_eq!(logs.len(), 2, "logs: {logs:?}"); + let implement = logs + .iter() + .find(|name| name.ends_with("-implement.jsonl")) + .expect("no implement log"); + let prefix = implement.strip_suffix("implement.jsonl").unwrap(); + assert!( + prefix.starts_with("acme-widgets-issue-7-"), + "log: {implement}" + ); + assert!( + logs.contains(&format!("{prefix}repair-1.jsonl")), + "logs: {logs:?}" + ); +} + +#[test] +fn fails_when_the_conflict_repair_leaves_the_merge_unfinished() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}{}", + base_moves_on("feature.txt", "base feature") + )); + scenario.agent_does_in_session(2, "true"); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert_eq!(result.stdout, ""); + assert!( + result + .stderr + .contains("merge of origin/main is still in progress"), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn fails_when_the_conflict_repair_aborts_the_merge() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}{}", + base_moves_on("feature.txt", "base feature") + )); + scenario.agent_does_in_session(2, "git merge --abort"); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert_eq!(result.stdout, ""); + assert!( + result + .stderr + .contains("origin/main is not merged into issue-7"), + "stderr: {}", + result.stderr + ); +} diff --git a/tests/support/mod.rs b/tests/support/mod.rs index ae25228bc..d2a2ded12 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -4,13 +4,16 @@ //! Layout of a scenario's temp root: //! //! ```text -//! origin.git/ bare repo standing in for github.com// +//! origin.git/ bare repo standing in for github.com//; +//! it rejects non-fast-forward pushes, so no rebase or +//! force-push can reach it //! home/ $HOME: .gitconfig with identity and the insteadOf rule //! bin/ fake gh and claude, first on PATH //! tmp/ $TMPDIR, so leftover temp directories are visible //! work// the launch clone, origin https://github.com//.git //! gh-state.json fake GitHub state //! 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 //! ``` @@ -59,6 +62,10 @@ impl Scenario { &scenario.path(""), &["init", "--bare", "--initial-branch=main", "origin.git"], ); + git( + &scenario.origin_dir(), + &["config", "receive.denyNonFastForwards", "true"], + ); let seed = scenario.path("seed"); git( &scenario.path(""), @@ -103,6 +110,12 @@ impl Scenario { fs::write(self.path("claude-script.sh"), script).unwrap(); } + /// Script the fake agent's `session`-th session (1-based) differently + /// from the rest. + pub fn agent_does_in_session(&self, session: usize, script: &str) { + fs::write(self.path(&format!("claude-script.sh.{session}")), script).unwrap(); + } + pub fn run(&self, args: &[&str]) -> RunResult { let path = format!( "{}:{}", @@ -175,6 +188,11 @@ impl Scenario { ) } + /// Output of a git command in the origin repo. + pub fn origin_git(&self, args: &[&str]) -> String { + git(&self.origin_dir(), args) + } + /// Output of a git command in the launch clone. pub fn launch_git(&self, args: &[&str]) -> String { git(&self.launch_dir(), args)