diff --git a/CONTEXT.md b/CONTEXT.md index bef4aaa9b..d88d5130a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -44,6 +44,9 @@ The step at the end of a **Merge run** in which thirdshift itself merges the pul **Policy refusal**: A merge the **Self-merge** tried that failed, where the round of the Repair loop that followed found nothing to fix: the **Base branch** unchanged, no conflict, CI green or absent, the pull request mergeable. The cause is a repository setting or rule, such as merge commits disallowed or a review required. thirdshift never reads GitHub's error text to decide it. The **Merge run** is a **Failed run** that leaves the pull request ready for review. +**Declined CI fix**: +A CI-fix **Repair** that found nothing on the branch to fix, for example because the check also fails on the **Base branch**: once the Repair ends and the **Base branch** is merged in again, the head of the **Issue branch** is still the commit whose CI just failed. Going round again would only watch the same red CI, so the **Run** is a **Failed run** with the cause `CI red on and the Repair found nothing to fix on the branch`, rather than spending its remaining Repairs. + **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. For a **Merge run**, it is also a Run that ends without its pull request merged. 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, unless the pull request is ready, mergeable and green and only the **Self-merge** could not happen. diff --git a/README.md b/README.md index cb54ace0d..b30c41fbf 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ From a clone of the issue's repository, `thirdshift `: 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, 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 5 per Run. If the Base branch moves while CI runs, it merges it again and goes round, at most 5 times per Run. In a Merge run, each round also takes in **Foreign commits** first: see [Foreign commits in a Merge run](#foreign-commits-in-a-merge-run). +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 5 per Run. If a CI-fix Repair leaves the head unchanged, with no commit of its own and no Base branch move to merge, the Run ends as a [Failed run](#failed-runs), since CI would stay red on the same head. If the Base branch moves while CI runs, it merges it again and goes round, at most 5 times per Run. In a Merge run, each round also takes in **Foreign commits** first: see [Foreign commits in a Merge run](#foreign-commits-in-a-merge-run). 7. In a **Merge run** (`thirdshift merge `), does the **Self-merge**: once the pull request is open, ready for review, mergeable and green, thirdshift merges it into the Base branch with a merge commit, on exactly the head commit whose CI it watched (`gh pr merge --merge --match-head-commit `). It never uses GitHub's auto-merge ([ADR-0004](docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md)). A merge that fails goes back round step 6, within the same budgets, and is tried again on the new green head. If that round finds nothing to fix, the refusal is a **policy refusal**, such as merge commits being disallowed or a review being required. After the merge, thirdshift deletes the Issue branch on `origin`, and closes the issue if it is still open, with the comment `Closed by #, merged into by a thirdshift Merge run.` GitHub closes it on its own only for a merge into the repository's default branch, and then thirdshift leaves it alone. 8. 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. @@ -167,7 +167,7 @@ A Run is not idempotent: re-running builds on whatever is already on the branch, ## Failed runs -A **Failed run** is one that ends, including by Ctrl-C or a closed terminal, without an open pull request from its Issue branch that targets the Base branch, is mergeable and has passing CI, or, for a Merge run, without that pull request merged. Causes include the session exiting non-zero, no pull request or one with the wrong base, running out of Repairs, a session that still ends while waiting on background work after its Resume, and a Base branch that keeps moving while CI runs, or in a Merge run, an Issue branch that keeps getting Foreign commits. +A **Failed run** is one that ends, including by Ctrl-C or a closed terminal, without an open pull request from its Issue branch that targets the Base branch, is mergeable and has passing CI, or, for a Merge run, without that pull request merged. Causes include the session exiting non-zero, no pull request or one with the wrong base, running out of Repairs, a CI-fix Repair that finds nothing on the branch to fix (a **declined CI fix**: the head is unchanged after it, so CI would stay red), a session that still ends while waiting on background work after its Resume, and a Base branch that keeps moving while CI runs, or in a Merge run, an Issue branch that keeps getting Foreign commits. A Failed run: diff --git a/src/ci.rs b/src/ci.rs index 7b286f4d3..a7b1f171f 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -21,7 +21,7 @@ pub enum Ci { /// 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)]; + let short = short(sha); progress::step(format_args!( "waiting up to {}s for CI on {short}", grace.as_secs() @@ -65,6 +65,11 @@ pub fn watch(issue: &IssueUrl, sha: &str) -> Result { Ok(Ci::Failed(failed)) } +/// `sha` shortened to 7 characters, as in progress messages. +pub fn short(sha: &str) -> &str { + &sha[..sha.len().min(7)] +} + fn count(checks: &[Check], state: CheckState) -> usize { checks.iter().filter(|check| check.state == state).count() } diff --git a/src/run.rs b/src/run.rs index c949b416d..5f1cdc982 100644 --- a/src/run.rs +++ b/src/run.rs @@ -285,11 +285,15 @@ impl RepairLoop<'_> { /// meanwhile. Green or absent CI also goes round again if the Base branch /// moved while CI ran, or, in a Merge run, if Foreign commits arrived. /// Returns the head commit whose CI was last watched and found green or - /// absent. Fails once a Repair beyond `MAX_REPAIRS`, or a round beyond - /// `MAX_UPSTREAM_MOVES`, would be needed. + /// absent. Fails with a Declined CI fix if, after a CI-fix Repair and the + /// Base branch merged again, the head is still the one whose CI failed, + /// and once a Repair beyond `MAX_REPAIRS`, or a round + /// beyond `MAX_UPSTREAM_MOVES`, would be needed. fn run(&mut self, run_session: &mut impl FnMut(&str, &str) -> Result<()>) -> Result { let (issue, worktree, base, pr_url) = (self.issue, self.worktree, self.base, self.pr_url); let branch = worktree.branch(); + // The head whose red CI the last CI-fix Repair was given. + let mut handed_to_repair = None; loop { if self.goal == Goal::Merged { self.take_in_foreign_commits(run_session)?; @@ -302,6 +306,12 @@ impl RepairLoop<'_> { } worktree.push()?; let head = worktree.head()?; + if handed_to_repair.as_ref() == Some(&head) { + bail!( + "CI red on {} and the Repair found nothing to fix on the branch", + ci::short(&head) + ); + } match ci::watch(issue, &head)? { Ci::Absent | Ci::Passed => { if worktree.base_branch_moved(base)? { @@ -318,6 +328,7 @@ impl RepairLoop<'_> { &kind, &prompt::ci_fix_repair(issue, base, branch, pr_url, &failed), )?; + handed_to_repair = Some(head); } } } diff --git a/tests/ci.rs b/tests/ci.rs index 089be3002..6939bd9c6 100644 --- a/tests/ci.rs +++ b/tests/ci.rs @@ -1,5 +1,6 @@ //! 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 5 Repairs. +//! commit, hands red CI to a CI-fix Repair, and gives up after 5 Repairs, or +//! as soon as a CI-fix Repair makes no commit. mod support; @@ -162,6 +163,98 @@ fn red_ci_after_the_last_repair_is_a_failed_run() { scenario.assert_cleaned_up("issue-7"); } +/// The cause of a Failed run whose CI-fix Repair left `head`, CI red, as it +/// was. +fn declined_ci_fix(head: &str) -> String { + format!( + "CI red on {} and the Repair found nothing to fix on the branch", + &head[..7] + ) +} + +/// Assert that a Failed run's stderr ends by naming the `kind` session's log. +fn assert_session_log_is(result: &support::RunResult, kind: &str) { + let last = result.stderr.lines().last().unwrap_or_default(); + assert!( + last.starts_with("thirdshift: session log: ") && last.ends_with(&format!("-{kind}.jsonl")), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn a_ci_fix_repair_that_makes_no_commit_is_a_failed_run() { + let scenario = Scenario::new(); + scenario.agent_does(&format!("{AGENT_OPENS_PR}{}", checks_on_head(RED))); + // The Repair finds the failure isn't the branch's and commits nothing. + scenario.agent_does_in_session(2, "true\n"); + + 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"); + let watched = scenario.origin_git(&["rev-parse", "issue-7~1"]); + let cause = declined_ci_fix(watched.trim()); + assert!(result.stderr.contains(&cause), "stderr: {}", result.stderr); + assert!( + !result.stderr.contains("repairs exhausted"), + "stderr: {}", + result.stderr + ); + assert_eq!( + result.stderr.matches("starting Repair").count(), + 1, + "stderr: {}", + result.stderr + ); + assert_eq!( + scenario.claude_calls().len(), + 2, + "the implement session and 1 Repair" + ); + assert_session_log_is(&result, "repair-1"); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], true); + assert_eq!( + scenario.origin_log("issue-7"), + Some(vec![ + format!("thirdshift: failed run ({cause})"), + "Add feature".to_string(), + "Initial commit".to_string(), + ]) + ); + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn a_second_ci_fix_repair_that_makes_no_commit_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!("{}{}", commits_fix(1), checks_on_head(RED))); + // The second Repair commits nothing. + scenario.agent_does_in_session(3, "true\n"); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + let watched = scenario.origin_git(&["rev-parse", "issue-7~1"]); + assert!( + result.stderr.contains(&declined_ci_fix(watched.trim())), + "stderr: {}", + result.stderr + ); + assert_eq!( + scenario.claude_calls().len(), + 3, + "the implement session and 2 Repairs" + ); + assert_session_log_is(&result, "repair-2"); + assert_eq!( + scenario.origin_log("issue-7").unwrap()[1], + "Fix CI 1".to_string() + ); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], true); +} + /// 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 { @@ -183,6 +276,26 @@ fn resolves_conflict(file: &str) -> String { format!("echo resolved > {file}\ngit add {file}\ngit commit -q --no-edit\n") } +#[test] +fn a_ci_fix_repair_that_makes_no_commit_goes_round_if_the_base_branch_moved() { + let scenario = Scenario::new(); + scenario.agent_does(&format!("{AGENT_OPENS_PR}{}", checks_on_head(RED))); + // The Repair commits nothing, but main moves on meanwhile, so CI is + // watched on the merge commit instead, which has no checks. + scenario.agent_does_in_session(2, &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!( + result.stderr.contains("no CI checks appeared"), + "stderr: {}", + result.stderr + ); + scenario.assert_cleaned_up("issue-7"); +} + #[test] fn conflict_and_ci_fix_repairs_share_one_cap() { let scenario = Scenario::new();