diff --git a/CONTEXT.md b/CONTEXT.md index c7c00c768..7fa0d4f77 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -35,7 +35,7 @@ A **Standards finding** or **Spec finding** the agent chose not to fix. It is li One invocation of the factory on an **Issue URL**, from launch to cleanup. **Failed run**: -A **Run** that ends, including by interruption, without an open pull request from its **Issue branch** that targets the **Base branch**, is mergeable, and has passing CI. Its work is still pushed so nothing is lost, and its open pull request, if any, is converted back to a draft. +A **Run** that ends, including by interruption, without an open pull request from its **Issue branch** that targets the **Base branch**, is mergeable, and has passing CI. Its work is still pushed so nothing is lost (or, if the push fails, its worktree and local **Issue branch** are kept), and its open pull request, if any, is converted back to a draft. **Issue branch**: A branch a **Run** works on for one issue: `issue-` (the first), then `issue--branch-` for k ≥ 2. A number counts as used once its pull request is merged or closed, even if the branch itself was deleted. diff --git a/README.md b/README.md index d7f5163cd..f78dd94b2 100644 --- a/README.md +++ b/README.md @@ -20,9 +20,9 @@ From a clone of the issue's repository, `thirdshift `: 2. Picks the **Issue branch** (`issue-`, or `issue--branch-` once earlier ones are finished) and the **Base branch**, normally the branch you have checked out. 3. Creates a git worktree next to your clone, named `-`, so your own checkout is never touched. 4. Runs a headless Claude Code session in that worktree with the **Factory skills** loaded. The agent implements the issue, reviews its work against the Base branch, addresses the **Standards findings** and **Spec findings** it agrees with, and opens a ready-for-review pull request that lists every **Unaddressed finding** with a reason and closes the issue. -5. Takes over deterministically: pushes the Issue branch, checks through `gh` that the pull request exists, is open and targets the Base branch, and marks it ready for review (`gh pr ready`) if the agent left it as a draft. +5. Takes over deterministically: pushes the Issue branch, skipping your repo's git hooks since the session runs the tests and CI gates the pull request, checks through `gh` that the pull request exists, is open and targets the Base branch, and marks it ready for review (`gh pr ready`) if the agent left it as a draft. 6. Keeps the pull request mergeable and green: merges the Base branch in and watches CI, starting a **Repair** session for a merge conflict or failing checks, at most 3 per Run. If the Base branch moves while CI runs, it merges it again and goes round, at most 3 times per Run. -7. Cleans up: removes the worktree, the local Issue branch and the temporary plugin directory, whether the Run succeeded or not. +7. Cleans up: removes the worktree, the local Issue branch and the temporary plugin directory, whether the Run succeeded or not. The one exception is a **Failed run** whose work could not be pushed: see below. ### Status @@ -142,7 +142,7 @@ A Failed run: 1. Commits any uncommitted work as `thirdshift: failed run ()`, with a timestamp and the hostname, and pushes the Issue branch, so nothing is lost. If the branch has no changes against the Base branch, nothing is pushed. 2. Converts its open pull request, if any, back to a draft, so a pull request only claims to be ready when the factory stands behind it. The next successful Continuation marks it ready again. -3. Cleans up as usual, prints the reason to stderr and exits non-zero. +3. Cleans up as usual, prints the reason to stderr and exits non-zero. If the push failed, the worktree and local Issue branch are kept instead, and stderr names the branch, its head commit and the worktree path, so you can recover the work or push it by hand. Merges, never rebases or force-pushes: a branch worked on from several servers never loses history. diff --git a/docs/adr/0002-existing-issue-branch-means-continue.md b/docs/adr/0002-existing-issue-branch-means-continue.md index df82786ec..bd637a80b 100644 --- a/docs/adr/0002-existing-issue-branch-means-continue.md +++ b/docs/adr/0002-existing-issue-branch-means-continue.md @@ -1,6 +1,6 @@ # An existing issue branch means continue, not error -When `origin/issue-` already exists with no PR or an open PR, a **Run** checks out that branch and continues the work, and updates the open PR if there is one. It does not refuse to run and does not start over. This lets work on an issue move between servers and environments, and lets a **Failed run** (whose work is always pushed) be picked up where it stopped. If the branch's PR is merged or closed, the Run starts a fresh numbered branch (`issue--branch-2`, `-3`, …) instead, so finished work is never reopened. +When `origin/issue-` already exists with no PR or an open PR, a **Run** checks out that branch and continues the work, and updates the open PR if there is one. It does not refuse to run and does not start over. This lets work on an issue move between servers and environments, and lets a **Failed run** (whose work is pushed whenever origin accepts it) be picked up where it stopped. If the branch's PR is merged or closed, the Run starts a fresh numbered branch (`issue--branch-2`, `-3`, …) instead, so finished work is never reopened. ## Consequences diff --git a/src/failed_run.rs b/src/failed_run.rs index ee07a0206..21119be2d 100644 --- a/src/failed_run.rs +++ b/src/failed_run.rs @@ -33,10 +33,11 @@ impl From for FailedRun { /// Take the Run in `worktree` down the Failed run path: commit and push the /// work, and send an open PR back to draft. Problems along the way are -/// reported, not raised, so `error` is what the Run fails with. +/// reported, not raised, so `error` is what the Run fails with. The worktree +/// is cleaned up, or kept if its work may not have reached origin. pub fn fail( issue: &IssueUrl, - worktree: &Worktree, + worktree: Worktree, base: &str, log: &Path, error: anyhow::Error, @@ -54,9 +55,10 @@ pub fn fail( .next() .unwrap_or_default() .to_string(); - if let Err(problem) = commit_and_push(worktree, base, &reason) { + let pushed = commit_and_push(&worktree, base, &reason); + if let Err(problem) = &pushed { progress::step(format_args!( - "could not push the failed run's work: {problem:#}" + "could not push the failed run's work, so it may exist only locally: {problem:#}" )); } let pr_url = match open_pr_as_draft(issue, worktree.branch()) { @@ -68,6 +70,10 @@ pub fn fail( None } }; + // Last, since keeping the worktree lets go of it. + if pushed.is_err() { + worktree.keep(); + } FailedRun { error, pr_url, @@ -103,8 +109,7 @@ fn commit_and_push(worktree: &Worktree, base: &str, reason: &str) -> Result<()> "-m", &message, ])?; - git.run(&["push", "origin", worktree.branch()])?; - Ok(()) + worktree.push() } /// Convert the open PR for `branch`, if there is one, to a draft, and return diff --git a/src/git.rs b/src/git.rs index f070cb99b..51654e926 100644 --- a/src/git.rs +++ b/src/git.rs @@ -20,16 +20,21 @@ impl Git { &self.dir } - /// Run `git ` and return its trimmed stdout, failing with git's - /// stderr if it exits non-zero. + /// Run `git ` and return its trimmed stdout. If it exits non-zero, + /// fail with the last lines of its stderr and then of its stdout, where a + /// hook's explanation can end up. pub fn run(&self, args: &[&str]) -> Result { let output = self.output(args)?; if !output.status.success() { - bail!( - "git {} failed: {}", - args.join(" "), - String::from_utf8_lossy(&output.stderr).trim() - ); + let tail: Vec = [&output.stderr, &output.stdout] + .into_iter() + .flat_map(|stream| last_lines(stream)) + .collect(); + let mut message = format!("git {} failed", args.join(" ")); + if !tail.is_empty() { + message = format!("{message}: {}", tail.join("\n")); + } + bail!(message); } Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) } @@ -48,3 +53,109 @@ impl Git { .with_context(|| format!("could not run git {}", args.join(" "))) } } + +/// The most lines of each stream a failure reports. +const MAX_LINES: usize = 10; + +/// The last `MAX_LINES` non-empty lines of `stream`, trimmed. +fn last_lines(stream: &[u8]) -> Vec { + let text = String::from_utf8_lossy(stream); + let lines: Vec<&str> = text + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .collect(); + lines[lines.len().saturating_sub(MAX_LINES)..] + .iter() + .map(|line| line.to_string()) + .collect() +} + +#[cfg(test)] +mod tests { + use std::os::unix::fs::PermissionsExt; + + use super::*; + + /// A repository in a temp directory, with an identity and one commit, and + /// a bare repository beside it as its `origin`. No global or system + /// config is read. + fn repo_with_origin() -> (tempfile::TempDir, Git) { + let temp = tempfile::TempDir::new().unwrap(); + let isolated = |dir: &Path, args: &[&str]| { + let status = Command::new("git") + .args(args) + .current_dir(dir) + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_NOSYSTEM", "1") + .status() + .unwrap(); + assert!(status.success(), "git {args:?}"); + }; + isolated(temp.path(), &["init", "-q", "--bare", "origin.git"]); + isolated(temp.path(), &["init", "-q", "-b", "main", "work"]); + let work = temp.path().join("work"); + for (key, value) in [ + ("user.name", "Test Runner"), + ("user.email", "runner@example.com"), + ("remote.origin.url", "../origin.git"), + ] { + isolated(&work, &["config", key, value]); + } + isolated(&work, &["commit", "-q", "--allow-empty", "-m", "Initial"]); + (temp, Git::new(work)) + } + + #[test] + fn a_failure_includes_a_hooks_output_and_gits_own_error() { + let (_temp, git) = repo_with_origin(); + let hook = git.dir().join(".git/hooks/pre-push"); + std::fs::write(&hook, "#!/bin/sh\necho 'hook says no'\nexit 1\n").unwrap(); + std::fs::set_permissions(&hook, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let error = git + .run(&["push", "origin", "main"]) + .unwrap_err() + .to_string(); + + assert!(error.contains("hook says no"), "{error}"); + assert!(error.contains("failed to push some refs"), "{error}"); + } + + #[test] + fn a_failure_includes_stdout() { + let (_temp, git) = repo_with_origin(); + + let error = git + .run(&["-c", "alias.fail=!echo $((6 * 7)); exit 1", "fail"]) + .unwrap_err() + .to_string(); + + assert!(error.ends_with(": 42"), "{error}"); + } + + #[test] + fn a_failure_keeps_only_the_last_lines_of_a_stream() { + let (_temp, git) = repo_with_origin(); + + let error = git + .run(&["-c", "alias.fail=!seq 1 100; exit 1", "fail"]) + .unwrap_err() + .to_string(); + + assert!(error.contains("\n100"), "{error}"); + assert!(!error.contains("\n50\n"), "{error}"); + } + + #[test] + fn a_failure_with_nothing_on_either_stream_says_only_that_git_failed() { + let (_temp, git) = repo_with_origin(); + + let error = git + .run(&["-c", "alias.fail=!exit 1", "fail"]) + .unwrap_err() + .to_string(); + + assert_eq!(error, "git -c alias.fail=!exit 1 fail failed"); + } +} diff --git a/src/run.rs b/src/run.rs index 157ac3064..2f443f097 100644 --- a/src/run.rs +++ b/src/run.rs @@ -21,7 +21,9 @@ use crate::worktree::{Merge, Worktree}; /// Take `issue` to a ready PR and return the PR's URL. Any failure after the /// worktree exists goes through the Failed run path. The worktree, the local -/// Issue branch and the plugin directory are gone when this returns. +/// Issue branch and the plugin directory are gone when this returns, except +/// that a Failed run whose work did not reach origin keeps the worktree and +/// branch. 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")?); @@ -50,7 +52,7 @@ pub fn run(issue: &IssueUrl) -> Result { }; let mut log = session::log_path(issue, ×tamp, "implement")?; implement(issue, &worktree, &base, &prompt, ×tamp, &mut log) - .map_err(|error| failed_run::fail(issue, &worktree, &base, &log, error)) + .map_err(|error| failed_run::fail(issue, worktree, &base, &log, error)) } /// The implement session given `prompt`, the checks on the PR it opened or diff --git a/src/worktree.rs b/src/worktree.rs index 4bf570af8..b719448ca 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -1,5 +1,6 @@ //! The Run's worktree: a sibling of the launch repository, on the Issue -//! branch, removed together with the local Issue branch when dropped. +//! branch, removed together with the local Issue branch when dropped unless +//! it is kept. use std::path::{Path, PathBuf}; @@ -21,6 +22,8 @@ pub struct Worktree { launch: Git, branch: String, git: Git, + /// Leave the worktree and local Issue branch in place when dropped. + kept: bool, } impl Worktree { @@ -78,6 +81,7 @@ impl Worktree { launch: Git::new(root), branch: branch.to_string(), git: Git::new(path), + kept: false, }) } @@ -94,12 +98,23 @@ impl Worktree { } /// Push the Issue branch to origin (a no-op if it is already there). + /// The target repo's hooks are skipped: the session runs the tests and CI + /// gates the PR, so a local hook doesn't decide whether work reaches + /// origin. pub fn push(&self) -> Result<()> { progress::step(format_args!("pushing {}", self.branch)); - self.git.run(&["push", "origin", &self.branch])?; + self.git + .run(&["push", "--no-verify", "origin", &self.branch])?; Ok(()) } + /// Let go of the worktree without removing it or the local Issue branch, + /// for work that may exist nowhere else. Dropping it then says where they + /// are and the branch's head commit. + pub fn keep(mut self) { + self.kept = true; + } + /// The Issue branch's head commit. pub fn head(&self) -> Result { self.git.run(&["rev-parse", "HEAD"]) @@ -158,6 +173,16 @@ impl Worktree { impl Drop for Worktree { fn drop(&mut self) { let path = self.path().to_string_lossy().into_owned(); + if self.kept { + let head = self + .head() + .unwrap_or_else(|error| format!("an unknown commit ({error:#})")); + progress::step(format_args!( + "keeping the worktree {path} and local branch {} at {head}", + self.branch + )); + return; + } progress::step(format_args!( "cleaning up the worktree and local branch {}", self.branch diff --git a/tests/failed_run.rs b/tests/failed_run.rs index 50c0f4779..e764916be 100644 --- a/tests/failed_run.rs +++ b/tests/failed_run.rs @@ -196,3 +196,85 @@ fn assert_interrupt_fails_the_run(signal: &str) { Some("half done\n".to_string()) ); } + +#[test] +fn a_rejecting_pre_push_hook_in_the_target_repo_does_not_block_the_failure_push() { + let scenario = Scenario::new(); + scenario.launch_has_rejecting_pre_push_hook(); + scenario.agent_does(AGENT_LEAVES_WORK_AND_EXITS_3); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_failed(&scenario, &result, ""); + assert_eq!( + scenario.origin_log("issue-7").unwrap()[0], + "thirdshift: failed run (claude exited 3)" + ); +} + +#[test] +fn when_origin_rejects_the_failure_push_the_worktree_and_local_branch_are_kept() { + let scenario = Scenario::new(); + scenario.repo_has_hook( + &scenario.origin_dir(), + "pre-receive", + "#!/bin/sh\necho \"origin says no\"\nexit 1\n", + ); + scenario.agent_does(AGENT_LEAVES_WORK_AND_EXITS_3); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert!( + result.stderr.contains("origin says no"), + "stderr: {}", + result.stderr + ); + assert_kept(&scenario, &result); +} + +#[test] +fn when_origin_is_unreachable_for_the_failure_push_the_worktree_and_local_branch_are_kept() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "git remote set-url origin {}\n{AGENT_LEAVES_WORK_AND_EXITS_3}", + scenario.path("nowhere.git").display() + )); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_kept(&scenario, &result); +} + +/// What a Failed run whose work didn't reach origin shares: the failure +/// commit is on the local Issue branch in the worktree, which are both kept, +/// and a line on stderr says where. +fn assert_kept(scenario: &Scenario, result: &RunResult) { + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(scenario.origin_log("issue-7"), None); + let worktree = scenario.path("work/widgets-issue-7"); + assert_eq!( + std::fs::read_to_string(worktree.join("wip.txt")).unwrap(), + "half done\n" + ); + let head = scenario.launch_git(&["rev-parse", "issue-7"]); + let head = head.trim(); + assert_eq!( + scenario + .launch_git(&["log", "-1", "--format=%s", head]) + .trim(), + "thirdshift: failed run (claude exited 3)" + ); + let kept = result + .stderr + .lines() + .find(|line| line.contains("keeping")) + .unwrap_or_else(|| panic!("no line saying what was kept: {}", result.stderr)); + for detail in ["issue-7", head, worktree.to_str().unwrap()] { + assert!(kept.contains(detail), "{detail:?} missing from {kept:?}"); + } + assert!( + !result.stderr.contains("cleaning up"), + "stderr: {}", + result.stderr + ); +} diff --git a/tests/fresh_run.rs b/tests/fresh_run.rs index 96c5bd274..03685237a 100644 --- a/tests/fresh_run.rs +++ b/tests/fresh_run.rs @@ -26,6 +26,21 @@ fn prints_only_the_pr_url_and_exits_zero() { assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); } +#[test] +fn a_rejecting_pre_push_hook_in_the_target_repo_does_not_block_the_push() { + let scenario = Scenario::new(); + scenario.launch_has_rejecting_pre_push_hook(); + 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").unwrap()[0], + "Add feature".to_string() + ); +} + #[test] fn runs_claude_headless_in_auto_mode_in_a_sibling_worktree_on_the_issue_branch() { let scenario = Scenario::new(); diff --git a/tests/support/mod.rs b/tests/support/mod.rs index adfa89bcd..cf0047722 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -321,6 +321,30 @@ impl Scenario { assert_eq!(self.entries("tmp"), Vec::::new()); } + /// Install `script` as the `name` hook (e.g. `"pre-push"`) of the git + /// repository at `repo`, such as the launch clone or `origin.git`. + pub fn repo_has_hook(&self, repo: &Path, name: &str, script: &str) { + let hooks = if repo.join(".git").is_dir() { + repo.join(".git/hooks") + } else { + repo.join("hooks") + }; + fs::create_dir_all(&hooks).unwrap(); + let hook = hooks.join(name); + fs::write(&hook, script).unwrap(); + fs::set_permissions(&hook, fs::Permissions::from_mode(0o755)).unwrap(); + } + + /// Give the launch clone a `pre-push` hook that says "hook says no" and + /// rejects every push, as a target repo's local hook might. + pub fn launch_has_rejecting_pre_push_hook(&self) { + self.repo_has_hook( + &self.launch_dir(), + "pre-push", + "#!/bin/sh\necho \"hook says no\"\nexit 1\n", + ); + } + /// Output of a git command in the launch clone. pub fn launch_git(&self, args: &[&str]) -> String { git(&self.launch_dir(), args)