From 61b4b524e46ee89dd5ca0cf940d44d55a54b2bbf Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 1 Oct 2026 18:36:41 +0000 Subject: [PATCH 1/2] Releasing a Claim, and removing it once a Self-merge has left the issue closed A Run or a Spec run that fails with nothing on origin to take over, no Issue branch or Spec branch and no pull request, puts its issue's labels back as they were before the Claim. A Merge run takes in-progress off once the issue is closed. Either failing is a warning naming the command to run by hand. Co-Authored-By: Claude Opus 5.5 --- README.md | 21 ++++-- src/claim.rs | 154 +++++++++++++++++++++++++++++++++++++-- src/github.rs | 39 +++++++++- src/interrupt.rs | 6 ++ src/main.rs | 6 +- src/progress.rs | 6 ++ src/run.rs | 105 +++++++++++++++----------- tests/architect_run.rs | 5 ++ tests/command_surface.rs | 2 + tests/failed_run.rs | 127 ++++++++++++++++++++++++++++++++ tests/fakes/gh.rs | 34 ++++++++- tests/merge_run.rs | 80 ++++++++++++++++++++ tests/spec_run.rs | 74 +++++++++++++++++++ 13 files changed, 599 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index 7fc201b4f..74bb025fe 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ From a clone of the issue's repository, `thirdshift `: 4. Runs a headless Claude Code session in that worktree with the **Factory skills** in [`skills/`](skills/) loaded, started with one of the **Session prompts** in [`prompts/`](prompts/). 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. A failed check that also failed, under the same name, on the Base branch commit the head last merged in is an **Inherited failure**, not the branch's to fix: the CI-fix Repair is given only the branch's own failures, with the Inherited failures listed as not to fix. If every failed check is an Inherited failure, no Repair starts: the Run merges the Base branch again if it has moved since, and otherwise ends as a [Failed run](#failed-runs) that says to fix the Base branch first, unless it was given `base-fix` or the [User config](#user-config) sets `base.fix`, when it starts a [Base fix](#base-fix) first. A check that is pending, passed or missing on that Base branch commit is the branch's own, and thirdshift never triggers or waits for the Base branch's CI. 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, 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. +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. Once the issue is closed, either way, thirdshift takes `in-progress` off it, [removing the Claim](#when-the-claim-ends). 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. ### The Claim @@ -37,7 +37,17 @@ The **Claim** is the mark that the factory has taken an issue, so the issue list - A [Continuation](#continuation) on an issue that is already `in-progress` changes nothing and makes no request. One on an issue that isn't Claimed yet Claims it. - If the Claim can't be made, the Run stops there, before any work, with exit `1` and the cause: `could not make the Claim on #: `. -thirdshift doesn't take `in-progress` off again yet: the label stays when the Run ends, whatever its outcome. +#### When the Claim ends + +How the Run or the Spec run ends decides what becomes of its Claim: + +- **Released**, when it fails with nothing on `origin` to take over: no Issue branch there and no pull request from it, or for a Spec run, no Spec branch and no Spec PR. That is a session that fails before it changes anything, as on a usage limit or an expired login, a Ctrl-C before any work, or a worktree that can't be created. The issue's labels go back as they were before the Claim: `in-progress` comes off, and `ready-for-agent` goes back if the issue had it, and only then. So an outage costs a retry and doesn't strand the issue. stderr says `releasing the Claim on #: labelling it ready-for-agent, in place of in-progress`, or `releasing the Claim on #: removing in-progress`. +- **Kept**, on any other ending that leaves the issue open: a pull request ready for review, or a [Failed run](#failed-runs) that pushed its Issue branch or left a pull request. The issue stays `in-progress` until you relabel it, so a failure waits for you. +- **Removed**, in a Merge run, once the Self-merge has left the issue closed, whether thirdshift closed it or GitHub did: `in-progress` comes off, after `removing in-progress from issue #` on stderr. + +A release undoes only what the Claim changed, in one request that keeps the issue's other labels, any added during the Run included. An issue that was already `in-progress` when the Run started stays `in-progress`, and one whose `in-progress` someone took off during the Run is left as it is. A Claim that can't be released, or can't be removed after a merge, doesn't change how the Run ends: stderr gets a `warning:` line naming the command to run by hand. + +A process killed outright, with `kill -9` or by a power cut, releases nothing: the issue stays `in-progress` for you to relabel. ### Status @@ -138,7 +148,7 @@ It sends one [Run notification](#run-notifications), whatever the outcome. To ha - **stdout** carries only the pull request's URL: on success, and on a Failed run that leaves an open pull request, a draft or, after a policy refusal, one ready for review. The exit code tells the two apart, so script it as `url=$(thirdshift "$issue") && echo "ready: $url"`. - **stderr** carries everything else: errors, cleanup problems, and progress lines while sessions run. Each line starts with the local time it was printed, as in `thirdshift: 12:14:49 pushing issue-7`, so a quiet terminal shows how long the Run has been on its last step. A successful Run's last line names the pull request too: `PR is ready for review`, or `PR is merged` after a Merge run, followed only by a `warning:` line if a [Run notification](#run-notifications) can't be sent. -- **Exit code** `0` means the Run ended with a pull request the factory stands behind, merged in a Merge run. Once the Self-merge has merged, the Run succeeds even if deleting the Issue branch on `origin` or closing the issue then fails: the merge can't be undone, so each failed step is a `warning:` line on stderr naming the command to run by hand, and the Run still exits `0` with the URL on stdout. Ctrl-C likewise: before the merge it makes a Failed run, after it thirdshift finishes these steps and exits as merged. `2` means the command is one thirdshift can't use: the Issue URL is missing or isn't a GitHub Issue URL, there is an argument other than the URL and the Run flags, a Run flag is repeated or contradicts another, or `parallel` isn't followed by a whole number from 1 up; the error and the help text go to stderr, before any work. A [User config](#user-config) thirdshift can't use exits `1`, also before any work. Any other failure exits `1`. +- **Exit code** `0` means the Run ended with a pull request the factory stands behind, merged in a Merge run. Once the Self-merge has merged, the Run succeeds even if deleting the Issue branch on `origin`, closing the issue or removing `in-progress` from it then fails: the merge can't be undone, so each failed step is a `warning:` line on stderr naming the command to run by hand, and the Run still exits `0` with the URL on stdout. Ctrl-C likewise: before the merge it makes a Failed run, after it thirdshift finishes these steps and exits as merged. `2` means the command is one thirdshift can't use: the Issue URL is missing or isn't a GitHub Issue URL, there is an argument other than the URL and the Run flags, a Run flag is repeated or contradicts another, or `parallel` isn't followed by a whole number from 1 up; the error and the help text go to stderr, before any work. A [User config](#user-config) thirdshift can't use exits `1`, also before any work. Any other failure exits `1`. The other commands: @@ -366,6 +376,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. The exception is a Merge run's policy refusal: the pull request is ready, mergeable and green and only the Self-merge could not happen, so it stays ready for review, and no failure commit is pushed onto the head whose CI was watched. 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. +4. Releases its [Claim](#when-the-claim-ends) if it left nothing on `origin`: no Issue branch and no pull request. Otherwise the issue stays `in-progress`. Merges, never rebases or force-pushes: a branch worked on from several servers never loses history. @@ -389,7 +400,7 @@ In a [Spec run](#spec-runs), a Ticket's Run says this on its own stderr, relayed A **Spec** is an issue with sub-issues, its **Tickets**. `thirdshift ` on a Spec is a **Spec run**: it works through the Tickets in the order their GitHub "blocked by" links allow, each Ticket's **Run** a **Merge run** into the **Spec branch**, then leaves one **Spec PR** from the Spec branch into the Base branch ready for review ([ADR-0006](docs/adr/0006-spec-runs-merge-tickets-into-a-spec-branch.md)). An issue with no sub-issues is an ordinary Run. -A Spec run makes the [Claim](#the-claim) on the Spec, as a Run does on its issue: the Spec is labelled `in-progress`, in place of `ready-for-agent`. Its Tickets' Runs change no label on their Tickets. +A Spec run makes the [Claim](#the-claim) on the Spec, as a Run does on its issue: the Spec is labelled `in-progress`, in place of `ready-for-agent`. Its Tickets' Runs change no label on their Tickets. The Claim [ends](#when-the-claim-ends) as a Run's does: it is released if the Spec run fails with no Spec branch on `origin` and no Spec PR, removed once a Self-merge of the Spec PR has left the Spec closed, and kept otherwise. The Spec branch is pushed before the first Ticket starts, so a Spec run that got as far as its Tickets keeps its Claim. The Spec PR opens as a draft as soon as the first Ticket lands, titled from the Spec, with `Closes #` and a Tickets checklist: one line per Ticket, ticked once it is done, with its pull request, or saying it is running, failed, blocked or unready. thirdshift rewrites the checklist between its `` markers as each Ticket starts and ends, leaving the rest of the body as it is. Once every Ticket is done, the **Spec review** rewrites the body, and thirdshift puts the checklist back, appending it if the markers are gone, before marking the Spec PR ready. @@ -458,7 +469,7 @@ An Architect run: 6. Marks the plan ready, in one request: `needs-triage` is swapped for `ready-for-agent`, the plan is labelled `architect-plan`, the mark a later Architect run finds an open **Architect plan** by, and its other labels are kept. thirdshift adds that label, never the agent, and first creates it if the repository has none. A Spec's Tickets are left as the review labelled them, without `architect-plan`. 7. Dispatches the plan, unless given `--plan-only`, exactly as `thirdshift ` would from the same clone on the Architect run's Base branch: a [Spec run](#spec-runs) when the plan has sub-issues, a Run otherwise. Its Base branch is the Architect run's, so with `base ` it is ``, not the branch checked out. `merge` and `no-merge` apply to the Spec PR or the Run's pull request, `parallel ` to the Spec run, and `base-fix` and `no-base-fix` to whether that run may start a [Base fix](#base-fix), as if given to that command, and the [User config](#user-config) sets what they leave unsaid: `merge.always`, `spec.parallel`, `base.fix`, `launch.pull` and `logs.dir`. The Architecture review itself never watches CI, so a Base fix can only happen in the dispatched run. It makes the [Claim](#the-claim) as that command would too: once its checks pass, the plan's `ready-for-agent` is swapped for `in-progress`, and the plan keeps `architect-plan`. The one difference is that it sends no [Run notification](#run-notifications) of its own, whatever `email.always` says: the Architect run sends [the one](#one-run-notification). -The dispatched run's ending is the Architect run's: its exit code, its pull request's URL alone on stdout, and its last line on stderr, `PR is ready for review` or `PR is merged`. If it fails, the Architect run fails as that Failed run or Failed spec run does, with the cause and the session log on stderr and the pull request's URL on stdout if it left one. The plan stays open, `in-progress` and `architect-plan`, for `thirdshift ` to take up again: no later Architect run [retries it](#one-architect-plan-at-a-time). `parallel ` on a plan that is a single Ticket fails the same way as it does for `thirdshift ` on an issue that isn't a Spec: `parallel is only for a Spec, and # has no sub-issues`, exit `1`, before any implementing and before the Claim, with the plan left `ready-for-agent`, to run without it. +The dispatched run's ending is the Architect run's: its exit code, its pull request's URL alone on stdout, and its last line on stderr, `PR is ready for review` or `PR is merged`. If it fails, the Architect run fails as that Failed run or Failed spec run does, with the cause and the session log on stderr and the pull request's URL on stdout if it left one. The plan stays open and `architect-plan`, and `in-progress` unless the run left nothing on `origin` and so [released its Claim](#when-the-claim-ends), for `thirdshift ` to take up again: no later Architect run [retries it](#one-architect-plan-at-a-time). `parallel ` on a plan that is a single Ticket fails the same way as it does for `thirdshift ` on an issue that isn't a Spec: `parallel is only for a Spec, and # has no sub-issues`, exit `1`, before any implementing and before the Claim, with the plan left `ready-for-agent`, to run without it. With `--plan-only`, it instead exits `0` with the plan's URL alone on stdout, and `plan is ready for an agent` as stderr's last line. Read or edit the plan, then run it with `thirdshift `. The plan is labelled `architect-plan` here too, so until it is finished or closed the next Architect run is [skipped](#one-architect-plan-at-a-time). diff --git a/src/claim.rs b/src/claim.rs index e1c537cc0..ffc0a9ccc 100644 --- a/src/claim.rs +++ b/src/claim.rs @@ -1,10 +1,14 @@ //! The Claim: the mark that the factory has taken an issue, the label //! `in-progress` in place of `ready-for-agent`, so the issue list shows what a -//! Run or a Spec run is working on. +//! Run or a Spec run is working on. It is released when that run ends with +//! nothing on origin to take over, and removed once its Self-merge has left +//! the issue closed. use anyhow::{Context, Result}; +use crate::git::Git; use crate::github; +use crate::interrupt; use crate::issue::IssueUrl; use crate::progress; use crate::spec_run::READY_FOR_AGENT; @@ -16,25 +20,39 @@ const IN_PROGRESS: &str = "in-progress"; /// if the repository lacks it. const IN_PROGRESS_DESCRIPTION: &str = "A Claim: thirdshift has taken this issue"; +/// The Claim a Run or a Spec run made on its issue, with what making it +/// changed, which is what releasing it puts back. +pub struct Claim<'a> { + issue: &'a IssueUrl, + /// Whether making it added `in-progress`: the issue was not Claimed + /// already. + added_in_progress: bool, + /// Whether making it took `ready-for-agent` off the issue. + took_ready: bool, +} + /// Make the Claim on `issue`: label it `in-progress`, in place of /// `ready-for-agent` if it has that, in one request that keeps its other /// labels, having added `in-progress` to the repository if it lacks it. An /// issue already Claimed, `in-progress` and not `ready-for-agent`, is left as /// it is, with no request made. A failure names the Claim as what could not /// be made. -pub fn make(issue: &IssueUrl) -> Result<()> { +pub fn make(issue: &IssueUrl) -> Result> { label_in_progress(issue) .with_context(|| format!("could not make the Claim on #{}", issue.number)) } /// [`make`], its failure as `gh` gave it. -fn label_in_progress(issue: &IssueUrl) -> Result<()> { +fn label_in_progress(issue: &IssueUrl) -> Result> { let mut labels = github::issue_labels(issue)?; - // GitHub's label names are case-insensitive. - let has = |label: &str| labels.iter().any(|name| name.eq_ignore_ascii_case(label)); - let (claimed, ready) = (has(IN_PROGRESS), has(READY_FOR_AGENT)); + let (claimed, ready) = (has(&labels, IN_PROGRESS), has(&labels, READY_FOR_AGENT)); + let claim = Claim { + issue, + added_in_progress: !claimed, + took_ready: ready, + }; if claimed && !ready { - return Ok(()); + return Ok(claim); } if ready { progress::step(format_args!( @@ -51,5 +69,125 @@ fn label_in_progress(issue: &IssueUrl) -> Result<()> { )?; } labels.retain(|name| !name.eq_ignore_ascii_case(READY_FOR_AGENT)); - github::set_labels_adding(issue, &labels, &[IN_PROGRESS]) + github::set_labels_adding(issue, &labels, &[IN_PROGRESS])?; + Ok(claim) +} + +/// Whether `label` is one of `labels`: GitHub's label names are +/// case-insensitive. +fn has(labels: &[String], label: &str) -> bool { + labels.iter().any(|name| name.eq_ignore_ascii_case(label)) +} + +impl Claim<'_> { + /// Release the Claim if the Run or the Spec run that made it ended with + /// nothing on origin to take over: neither `branch`, its Issue branch or + /// Spec branch, which `launch` asks origin for, nor a pull request from + /// it. The issue's labels then go back as they were before the Claim: + /// `in-progress` comes off if the Claim added it, and `ready-for-agent` + /// goes back if the Claim took it off, in one request that keeps the + /// issue's other labels, those added since included. An issue that is no + /// longer `in-progress` is left as it is: someone took the Claim off + /// meanwhile. A Claim that changed no label has nothing to put back, and + /// makes no request. + /// + /// The run has ended as it has, so this never fails: a failure is a + /// warning naming what to run by hand, and an interrupt doesn't stop it. + pub fn release_if_nothing_on_origin(&self, launch: &Git, branch: &str) { + if !self.added_in_progress && !self.took_ready { + return; + } + if let Err(error) = interrupt::retry(|| self.release_unless_on_origin(launch, branch)) { + progress::warn( + &error, + format_args!( + "could not release the Claim on #{}, so if nothing of the run is on origin, \ + release it by hand: {}", + self.issue.number, + self.release_by_hand() + ), + ); + } + } + + /// [`Claim::release_if_nothing_on_origin`], its failure as `git` or `gh` + /// gave it. + fn release_unless_on_origin(&self, launch: &Git, branch: &str) -> Result<()> { + let issue = self.issue; + let reference = format!("refs/heads/{branch}"); + let on_origin = launch.run(&["ls-remote", "--heads", "origin", &reference])?; + if !on_origin.is_empty() || github::pull_request_for(issue, branch)?.is_some() { + return Ok(()); + } + let mut labels = github::issue_labels(issue)?; + if !has(&labels, IN_PROGRESS) { + return Ok(()); + } + let number = issue.number; + if !self.took_ready { + progress::step(format_args!( + "releasing the Claim on #{number}: removing {IN_PROGRESS}" + )); + return github::remove_label(issue, IN_PROGRESS); + } + if self.added_in_progress { + progress::step(format_args!( + "releasing the Claim on #{number}: labelling it {READY_FOR_AGENT}, \ + in place of {IN_PROGRESS}" + )); + labels.retain(|name| !name.eq_ignore_ascii_case(IN_PROGRESS)); + } else { + progress::step(format_args!( + "releasing the Claim on #{number}: labelling it {READY_FOR_AGENT} again" + )); + } + github::set_labels_adding(issue, &labels, &[READY_FOR_AGENT]) + } + + /// Remove the Claim once the Self-merge of the Run or the Spec run that + /// made it has left its issue closed: `in-progress` comes off, in one + /// request that keeps the issue's other labels. An issue still open, as + /// when closing it failed, keeps it, and one no longer `in-progress` is + /// left as it is. + /// + /// Like the other steps after a merge, this never fails: a failure is a + /// warning naming the command to run by hand, and an interrupt doesn't + /// stop it. + pub fn remove_if_closed(&self) { + if let Err(error) = interrupt::retry(|| self.remove_label_if_closed()) { + progress::warn( + &error, + format_args!( + "could not remove {IN_PROGRESS} from issue #{}, so remove it by hand: {}", + self.issue.number, + github::remove_label_command(self.issue, IN_PROGRESS) + ), + ); + } + } + + /// [`Claim::remove_if_closed`], its failure as `gh` gave it. + fn remove_label_if_closed(&self) -> Result<()> { + let issue = github::issue(self.issue)?; + if issue.is_open || !has(&issue.labels, IN_PROGRESS) { + return Ok(()); + } + progress::step(format_args!( + "removing {IN_PROGRESS} from issue #{}", + self.issue.number + )); + github::remove_label(self.issue, IN_PROGRESS) + } + + /// The commands that release the Claim by hand. + fn release_by_hand(&self) -> String { + let mut commands = Vec::new(); + if self.added_in_progress { + commands.push(github::remove_label_command(self.issue, IN_PROGRESS)); + } + if self.took_ready { + commands.push(github::add_label_command(self.issue, READY_FOR_AGENT)); + } + commands.join(" && ") + } } diff --git a/src/github.rs b/src/github.rs index 70a7279eb..d370cb5d2 100644 --- a/src/github.rs +++ b/src/github.rs @@ -43,7 +43,7 @@ pub fn issue_title(issue: &IssueUrl) -> Result { } /// An issue as an Architect run reads the plan its Architecture review -/// published. +/// published, and as a Claim reads its issue after a Self-merge. pub struct Issue { pub is_open: bool, pub labels: Vec, @@ -110,6 +110,43 @@ pub fn set_labels_adding(issue: &IssueUrl, kept: &[String], added: &[&str]) -> R set_labels(issue, &labels) } +/// The REST API's path for `label` on `issue`. +fn label_path(issue: &IssueUrl, label: &str) -> String { + format!( + "repos/{}/issues/{}/labels/{label}", + issue.repo_slug(), + issue.number + ) +} + +/// Take `label` off `issue`, in one request that keeps its other labels. +/// Through the REST API, as [`set_labels`] is. GitHub refuses it if the issue +/// does not have the label. +pub fn remove_label(issue: &IssueUrl, label: &str) -> Result<()> { + gh(&[ + "api", + "--method", + "DELETE", + &label_path(issue, label), + "--silent", + ]) +} + +/// The command that does what [`remove_label`] does, to run by hand. +pub fn remove_label_command(issue: &IssueUrl, label: &str) -> String { + format!("gh api --method DELETE {}", label_path(issue, label)) +} + +/// The command that adds `label` to `issue`, keeping its other labels, to +/// run by hand. +pub fn add_label_command(issue: &IssueUrl, label: &str) -> String { + format!( + "gh api --method POST repos/{}/issues/{}/labels -f 'labels[]={label}'", + issue.repo_slug(), + issue.number + ) +} + /// A Spec's sub-issue, as a Spec run reads it. pub struct Ticket { pub number: u64, diff --git a/src/interrupt.rs b/src/interrupt.rs index 904dc0f58..05b845033 100644 --- a/src/interrupt.rs +++ b/src/interrupt.rs @@ -27,6 +27,12 @@ pub fn requested() -> bool { .is_some_and(|flag| flag.load(Ordering::SeqCst)) } +/// Run `step`, and once more if it failed with the Run interrupted: Ctrl-C in +/// a terminal also kills the git or gh the step was running. +pub fn retry(step: impl Fn() -> Result<()>) -> Result<()> { + step().or_else(|error| if requested() { step() } else { Err(error) }) +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/main.rs b/src/main.rs index d887c5efe..e547ab9ea 100644 --- a/src/main.rs +++ b/src/main.rs @@ -80,7 +80,11 @@ A Run or a Spec run makes the Claim on its issue once its checks pass, before an labels the issue in-progress, in place of ready-for-agent if it has that, keeping its other labels and creating the label if the repository lacks it. A Ticket's Run in a Spec run and a Base fix make none, and an issue already in-progress is left as it is. A Run whose Claim -can't be made stops there, naming the cause. +can't be made stops there, naming the cause. The Claim is released, the issue's labels put +back as they were, when the Run or the Spec run fails with nothing on origin to take over: +no Issue branch or Spec branch and no pull request. It is removed once a Self-merge has left +the issue closed, and otherwise stays: a failure to release or remove it is a warning naming +the command to run by hand. On a Spec, an issue with sub-issues, the Run is a Spec run: it takes every Ticket (sub-issue) it can reach, in the order their \"blocked by\" links allow, each merged into the Spec branch. Its Spec diff --git a/src/progress.rs b/src/progress.rs index 5be0dd9ba..01c5cbe5e 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -24,6 +24,12 @@ pub fn step(message: impl Display) { write_line(Local::now().format(STAMP), message); } +/// Report `error`, then a warning saying what to do about it by hand. +pub fn warn(error: &anyhow::Error, warning: std::fmt::Arguments) { + step(format_args!("{error:#}")); + step(format_args!("warning: {warning}")); +} + /// Print `line`, from the stderr of a child thirdshift, under `label`, /// keeping the time the child stamped it with (or stamping it now, if it /// has none), and return its message: the line without prefix or time. diff --git a/src/run.rs b/src/run.rs index 06f1a7057..0ff120341 100644 --- a/src/run.rs +++ b/src/run.rs @@ -1,6 +1,7 @@ //! One Run: from an Issue URL to a checked PR, or to a Failed run. An issue //! with sub-issues is a Spec instead, handed to a Spec run. +use std::num::NonZeroUsize; use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; @@ -12,7 +13,7 @@ use crate::ci::{self, Ci}; use crate::claim; use crate::failed_run::{self, FailedRun, PolicyRefusal}; use crate::git::Git; -use crate::github::{self, Mergeable, PullRequest}; +use crate::github::{self, Mergeable, PullRequest, Ticket}; use crate::interrupt; use crate::issue::IssueUrl; use crate::plugin::Plugin; @@ -154,7 +155,9 @@ pub fn run_to_end( /// /// Once those checks pass, and before the worktree is created, the Run, or /// the Spec run, makes the Claim on `issue`, unless it is a child Run. A Claim -/// that can't be made fails it there, before any work. +/// that can't be made fails it there, before any work. The Claim is released +/// if it then fails with nothing on origin to take over, and removed once its +/// Self-merge has left `issue` closed: see [`claim::Claim`]. /// /// `base_fix` is the one Base fix the Run, or a Spec run for its Spec PR, may /// start, or wait on, when its only red checks are Inherited failures. @@ -202,37 +205,69 @@ fn run( if interrupt::requested() { return Err(anyhow!("interrupted").into()); } - if started_by.makes_claim() { - claim::make(issue)?; + let claim = started_by + .makes_claim() + .then(|| claim::make(issue)) + .transpose()?; + let logs = Logs::of_run(issue, logs_dir, ×tamp); + let outcome = run_in_worktree( + issue, + tickets, + &launch, + &selection, + &base, + goal, + base_fix, + &logs, + parallel.tickets, + ); + if let Some(claim) = claim { + match &outcome { + Ok(reached) if reached.goal == Goal::Merged => claim.remove_if_closed(), + // Ready for review: the Claim stays while the pull request waits. + Ok(_) => {} + Err(_) => claim.release_if_nothing_on_origin(&launch, &branch), + } } - let worktree = match &selection { - Selection::Fresh { .. } => Worktree::create_fresh(&launch, &issue.repo, &branch, &base)?, + outcome +} + +/// [`run`], from the worktree on: create it in the Launch directory `launch` +/// for the branch `selection` picked, and take `issue` to `goal` there, as a +/// Spec run if it has `tickets`. +#[allow(clippy::too_many_arguments)] +fn run_in_worktree( + issue: &IssueUrl, + tickets: Vec, + launch: &Git, + selection: &Selection, + base: &str, + goal: Goal, + base_fix: &mut BaseFix, + logs: &Logs, + parallel: NonZeroUsize, +) -> Result { + let branch = selection.branch(); + let worktree = match selection { + Selection::Fresh { .. } => Worktree::create_fresh(launch, &issue.repo, branch, base)?, Selection::Continuation { .. } => { - Worktree::continue_existing(&launch, &issue.repo, &branch, &base)? + Worktree::continue_existing(launch, &issue.repo, branch, base)? } }; - let logs = Logs::of_run(issue, logs_dir, ×tamp); if !tickets.is_empty() { return spec_run::run( - issue, - tickets, - worktree, - &base, - goal, - base_fix, - &logs, - parallel.tickets, + issue, tickets, worktree, base, goal, base_fix, logs, parallel, ); } - let prompt = match &selection { - Selection::Fresh { .. } => prompt::fresh(issue, &base, &branch), + let prompt = match selection { + Selection::Fresh { .. } => prompt::fresh(issue, base, branch), Selection::Continuation { pr, .. } => { - prompt::continuation(issue, &base, &branch, pr.as_ref().map(|pr| pr.url.as_str())) + prompt::continuation(issue, base, branch, pr.as_ref().map(|pr| pr.url.as_str())) } }; let mut log = logs.path("implement"); let implemented = implement( - issue, &worktree, &base, &prompt, goal, base_fix, &logs, &mut log, + issue, &worktree, base, &prompt, goal, base_fix, logs, &mut log, ); match implemented { Ok(pr_url) => Ok(Reached { @@ -241,7 +276,7 @@ fn run( log: Some(log), ticket_lines: Vec::new(), }), - Err(error) => Err(failed_run::fail(issue, worktree, &base, &log, error)), + Err(error) => Err(failed_run::fail(issue, worktree, base, &log, error)), } } @@ -264,7 +299,7 @@ pub fn pull_base_branch(launch: &Git, checked_out: Option<&str>, base: &str) { "updating {base} in the Launch directory from {origin_base}" )); if let Err(error) = launch.run(&["merge", "--ff-only", "--quiet", &origin_base]) { - warn( + progress::warn( &error, format_args!( "could not update {base} in the Launch directory, \ @@ -362,8 +397,8 @@ pub fn deliver( /// hand. fn after_merge(issue: &IssueUrl, worktree: &Worktree, pr: &PullRequest) { let branch = worktree.branch(); - if let Err(error) = retry_if_interrupted(|| worktree.delete_from_origin()) { - warn( + if let Err(error) = interrupt::retry(|| worktree.delete_from_origin()) { + progress::warn( &error, format_args!( "could not delete {branch} on origin, so delete it by hand: \ @@ -375,8 +410,8 @@ fn after_merge(issue: &IssueUrl, worktree: &Worktree, pr: &PullRequest) { "Closed by #{}, merged into {} by a thirdshift Merge run.", pr.number, pr.base ); - if let Err(error) = retry_if_interrupted(|| close_unless_closed(issue, &comment)) { - warn( + if let Err(error) = interrupt::retry(|| close_unless_closed(issue, &comment)) { + progress::warn( &error, format_args!( "could not close issue #{number}, so if it is still open, close it by hand: \ @@ -398,24 +433,6 @@ fn close_unless_closed(issue: &IssueUrl, comment: &str) -> Result<()> { github::close_issue(issue, comment) } -/// Run `step`, and once more if it failed with the Run interrupted: Ctrl-C in -/// a terminal also kills the git or gh the step was running. -fn retry_if_interrupted(step: impl Fn() -> Result<()>) -> Result<()> { - step().or_else(|error| { - if interrupt::requested() { - step() - } else { - Err(error) - } - }) -} - -/// Report `error`, then a warning saying what to do about it by hand. -fn warn(error: &anyhow::Error, warning: std::fmt::Arguments) { - progress::step(format_args!("{error:#}")); - progress::step(format_args!("warning: {warning}")); -} - /// The most Repair sessions a Run starts, conflict, CI-fix and review /// combined. const MAX_REPAIRS: usize = 5; diff --git a/tests/architect_run.rs b/tests/architect_run.rs index c3e2163b1..fa2868079 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -2080,6 +2080,11 @@ fn a_plan_whose_dispatched_run_failed_stays_open_and_the_next_architect_run_neve scenario.agent_does_for(8, "exit 3"); let first = scenario.run(&["architect"]); assert_eq!(first.code, Some(1), "stderr: {}", first.stderr); + // The run left nothing on origin, so its Claim on the plan is released. + assert_eq!( + scenario.issue_labels(8), + [ARCHITECT_PLAN, "ready-for-agent"] + ); let sessions = scenario.claude_calls().len(); let second = scenario.run(&["architect"]); diff --git a/tests/command_surface.rs b/tests/command_surface.rs index b2bf3811c..47ceb8717 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -214,6 +214,8 @@ fn help_documents_the_claim() { "creating the label if the repository lacks it", "A Ticket's Run in a Spec run and a Base fix make none", "A Run whose Claim can't be made stops there", + "The Claim is released, the issue's labels put back as they were, when the Run or the Spec run fails with nothing on origin to take over: no Issue branch or Spec branch and no pull request", + "It is removed once a Self-merge has left the issue closed, and otherwise stays", "That run makes the Claim on the plan, which keeps architect-plan", ] { assert!(help.contains(mention), "help lacks {mention:?}: {help}"); diff --git a/tests/failed_run.rs b/tests/failed_run.rs index 1c6bf0359..116b62073 100644 --- a/tests/failed_run.rs +++ b/tests/failed_run.rs @@ -276,3 +276,130 @@ fn assert_kept(scenario: &Scenario, result: &RunResult) { result.stderr ); } + +#[test] +fn a_failed_run_that_pushed_nothing_releases_its_claim() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["bug", "ready-for-agent", "architecture"]); + scenario.agent_does("exit 1"); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_failed(&scenario, &result, ""); + assert_eq!(scenario.origin_log("issue-7"), None); + assert_eq!( + scenario.issue_labels(7), + ["bug", "architecture", "ready-for-agent"] + ); + assert!( + result.stderr.contains( + "thirdshift: releasing the Claim on #7: labelling it ready-for-agent, in place of in-progress\n" + ), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn a_released_claim_gives_no_ready_for_agent_to_an_issue_that_had_none() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["bug"]); + scenario.agent_does("exit 1"); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_failed(&scenario, &result, ""); + assert_eq!(scenario.issue_labels(7), ["bug"]); + assert!( + result + .stderr + .contains("thirdshift: releasing the Claim on #7: removing in-progress\n"), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn an_interrupted_run_that_left_nothing_on_origin_releases_its_claim() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does(&format!( + "touch {}\nsleep 30\n", + scenario.path("agent-started").display() + )); + + let result = scenario.run_and_signal(&[&scenario.issue_url(7)], "agent-started", "INT"); + + assert_failed(&scenario, &result, ""); + assert_eq!(scenario.origin_log("issue-7"), None); + assert_eq!(scenario.issue_labels(7), ["ready-for-agent"]); +} + +#[test] +fn a_failed_run_that_pushed_its_issue_branch_keeps_its_claim() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does(AGENT_LEAVES_WORK_AND_EXITS_3); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_failed(&scenario, &result, ""); + assert!(scenario.origin_log("issue-7").is_some()); + assert_eq!(scenario.issue_labels(7), ["in-progress"]); + assert!( + !result.stderr.contains("releasing the Claim"), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn a_released_claim_keeps_a_label_the_issue_was_given_during_the_run() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does("gh fake labels 7 '[\"in-progress\", \"needs-info\"]'\nexit 1\n"); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_failed(&scenario, &result, ""); + assert_eq!(scenario.issue_labels(7), ["needs-info", "ready-for-agent"]); +} + +#[test] +fn an_issue_whose_claim_someone_took_off_during_the_run_is_left_as_it_is() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does("gh fake labels 7 '[\"ready-for-human\"]'\nexit 1\n"); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_failed(&scenario, &result, ""); + assert_eq!(scenario.issue_labels(7), ["ready-for-human"]); + assert!( + !result.stderr.contains("releasing the Claim"), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn a_claim_that_cannot_be_released_is_a_warning_naming_the_commands_to_run_by_hand() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does("gh fake fails 'api --method'\nexit 1\n"); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_failed(&scenario, &result, ""); + assert_eq!(scenario.issue_labels(7), ["in-progress"]); + assert!( + result.stderr.contains( + "thirdshift: warning: could not release the Claim on #7, so if nothing of the run \ + is on origin, release it by hand: \ + gh api --method DELETE repos/acme/widgets/issues/7/labels/in-progress && \ + gh api --method POST repos/acme/widgets/issues/7/labels -f 'labels[]=ready-for-agent'\n" + ), + "stderr: {}", + result.stderr + ); +} diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index b1d5a354d..2dc2a839e 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -42,7 +42,10 @@ //! `createdAt` its entry in `created`, else the first second of 2020. //! //! `gh api --method PUT repos//issues//labels -f -//! labels[]=