diff --git a/CONTEXT.md b/CONTEXT.md index 793d41780..1a3f96931 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -20,7 +20,7 @@ _Avoid_: originating workspace, launch repo The rule that the **Issue URL** must belong to the same GitHub repository as the `origin` remote of the **Launch directory**. A mismatch stops the run before any work happens. **Base branch**: -The branch the work branches off and the pull request targets. Normally the branch checked out in the **Launch directory**; for an **Architect run** whose command names a branch, that branch, whatever is checked out; for a **Ticket**'s **Run** in a **Spec run**, the **Spec branch**; in a **Continuation** with an open pull request, that pull request's base instead. +The branch the work branches off and the pull request targets. Normally the branch checked out in the **Launch directory**; for an **Architect run** or a **Pickup run** whose command names a branch, that branch, whatever is checked out; for a **Ticket**'s **Run** in a **Spec run**, the **Spec branch**; in a **Continuation** with an open pull request, that pull request's base instead. **Factory skills**: The skills in `skills/`, loaded into the session as the `thirdshift` plugin: adapted copies of Matt Pocock's skills, designed to run headless with no human in the loop. diff --git a/README.md b/README.md index 7fc201b4f..47a69f9bc 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ The **Claim** is the mark that the factory has taken an issue, so the issue list - It is made once every check of step 1 has passed and the branches are picked, and before the worktree is created. A Run stopped by a check changes no label. - It is one request, which adds `in-progress`, removes `ready-for-agent` and keeps the issue's other labels, so the swap can't stop halfway. stderr says `labelling # in-progress, in place of ready-for-agent`, or `labelling # in-progress`. - thirdshift first creates the `in-progress` label if the repository has none. -- It is made however the Run or Spec run was started: by you on an Issue URL, or by an [Architect run](#architect-runs) for the plan it dispatches. +- It is made however the Run or Spec run was started: by you on an Issue URL, by an [Architect run](#architect-runs) for the plan it dispatches, or by a [Pickup run](#pickup-runs) for the Ready issue it took. - A Ticket's Run in a Spec run makes none, since the Spec carries the Claim, and nor does a [Base fix](#base-fix): their issues' labels are left as they are. - 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 #: `. @@ -146,6 +146,8 @@ The other commands: thirdshift architect [] # review the Base branch's architecture, publish a plan and run it (see Architect runs) thirdshift architect [] --plan-only # publish the plan, mark it ready and stop there thirdshift architect base [] # do either with as the Base branch, from a clone on any branch +thirdshift pickup # take the lowest-numbered Ready issue in the repository and run it (see Pickup runs) +thirdshift pickup base # do that with as the Base branch, from a clone on any branch thirdshift email-test [
] # send a test email through Resend (see Email) thirdshift setup # choose your defaults and write the User config with every setting (see User config) thirdshift update # update to the latest release (see Updating) @@ -153,7 +155,7 @@ thirdshift version # print thirdshift thirdshift help # print every form of the command, each with a one-line description ``` -`version` and `help` print to stdout and exit `0`. `update`, `setup` and `email-test` follow the Run's rule: stdout stays empty, messages go to stderr. `architect` follows it too: stdout carries the pull request's URL of the run it dispatches, or, in its place, the URL of the issue it ended on: its plan with `--plan-only`, or its idea when the review published no plan. A skipped Architect run prints nothing there when [another is still running](#one-at-a-time), and the URL of each Architect plan that is [still open](#one-architect-plan-at-a-time). +`version` and `help` print to stdout and exit `0`. `update`, `setup` and `email-test` follow the Run's rule: stdout stays empty, messages go to stderr. `architect` follows it too: stdout carries the pull request's URL of the run it dispatches, or, in its place, the URL of the issue it ended on: its plan with `--plan-only`, or its idea when the review published no plan. A skipped Architect run prints nothing there when [another is still running](#one-at-a-time), and the URL of each Architect plan that is [still open](#one-architect-plan-at-a-time). `pickup` follows it as well: stdout carries the pull request's URL of the run it dispatches, and nothing when the [Pickup run](#pickup-runs) is skipped. Uncommitted changes in your clone are fine: the Run works in its own worktree from `origin`, so they are simply left out. Unpushed commits on the Base branch are not: push them first, or the Run stops. @@ -281,6 +283,8 @@ A [Spec run](#spec-runs) sends at most one notification for the whole Spec, unde An [Architect run](#architect-runs) takes the same flags and the same `email.always`, and sends [one notification](#one-run-notification) covering its review and the run it dispatched. +A [Pickup run](#pickup-runs) takes them too, for the run it dispatches, which sends its notification as it would by hand. A skipped Pickup run sends none. + A notification that can't be sent is a `warning:` line on stderr with Resend's error. It never changes the Run's outcome, stdout or exit code. ### Logs @@ -444,13 +448,13 @@ The focus is optional free text, given as one argument, anywhere among the flags `merge`, `no-merge`, `base-fix`, `no-base-fix` and `parallel ` (or `--merge`, `--no-merge`, `--base-fix`, `--no-base-fix` and `--parallel `) are for the run the plan is dispatched as, and mean what they do for `thirdshift `, so none of them is ever read as the focus. `--plan-only` stops the Architect run once the plan is marked ready, and dispatches nothing. `email`, optionally followed by an address, and `no-email` (or `--email` and `--no-email`) are for the Architect run's own [Run notification](#one-run-notification), with or without `--plan-only`. The word after `email` is the address only if it contains `@`, so a focus without one is never taken for it. -`base ` (or `--base `) names the Architect run's **Base branch**, so the branch your clone has checked out stops mattering: start it from a clone on another branch, on a detached HEAD, or with uncommitted changes. The Architecture review's worktree starts at the head of `` on `origin`, and the run the plan is dispatched as takes `` as its Base branch too: a Run's **Issue branch**, or a Spec run's **Spec branch**, is branched off it, and the pull request or **Spec PR** targets it. It goes before or after the focus and the other flags, `--plan-only` included, and the word after it is always the branch, never the focus. Without `base`, the Base branch is the branch checked out. `base` is a flag of `architect` only: `thirdshift ` doesn't take it. It is what lets an Architect run be started [on a schedule](#on-a-schedule). +`base ` (or `--base `) names the Architect run's **Base branch**, so the branch your clone has checked out stops mattering: start it from a clone on another branch, on a detached HEAD, or with uncommitted changes. The Architecture review's worktree starts at the head of `` on `origin`, and the run the plan is dispatched as takes `` as its Base branch too: a Run's **Issue branch**, or a Spec run's **Spec branch**, is branched off it, and the pull request or **Spec PR** targets it. It goes before or after the focus and the other flags, `--plan-only` included, and the word after it is always the branch, never the focus. Without `base`, the Base branch is the branch checked out. `base` is a flag of `architect` and [`pickup`](#pickup-runs) only: `thirdshift ` doesn't take it. It is what lets an Architect run be started [on a schedule](#on-a-schedule). A second focus, a repeated flag, `merge` with `no-merge`, `base-fix` with `no-base-fix`, `email` with `no-email`, `parallel` without a whole number from 1 up, `base` with no branch after it, any other argument starting with a dash, or an empty focus is an argument error (exit `2`). So is `merge`, `no-merge`, `base-fix`, `no-base-fix` or `parallel` with `--plan-only`, since nothing is dispatched for them to apply to. An Architect run: -1. Makes the checks a Run makes that don't need an issue, before creating anything: `origin` is a GitHub repository, git has a `user.name` and `user.email`, HEAD is not detached, and the Base branch exists on `origin` with your local copy not ahead of it. With `base `, a detached HEAD is fine, since the checkout no longer picks the Base branch, and the other two checks are made on ``: `base branch does not exist on origin; push it first`, or `local is commit(s) ahead of origin/; push them first`. There is no **Origin match**, since there is no Issue URL: the repository is the one `origin` names. If another Architect run on the repository is still running, it is [skipped](#one-at-a-time) here, and if not, it is skipped when an earlier [Architect plan is still open](#one-architect-plan-at-a-time). With `launch.pull = true` in the [User config](#user-config), it then fast-forwards your checkout of the Base branch, as a Run does, and so only when the Base branch is the branch checked out: with `base ` from a clone on another branch, your checkout is left alone. +1. Makes the checks a Run makes that don't need an issue, before creating anything: `origin` is a GitHub repository, git has a `user.name` and `user.email`, HEAD is not detached, and the Base branch exists on `origin` with your local copy not ahead of it. With `base `, a detached HEAD is fine, since the checkout no longer picks the Base branch, and the other two checks are made on ``: `base branch does not exist on origin; push it first`, or `local is commit(s) ahead of origin/; push them first`. There is no **Origin match**, since there is no Issue URL: the repository is the one `origin` names. If another Architect run on the repository, or a [Pickup run](#pickup-runs), is still running, it is [skipped](#one-at-a-time) here, and if not, it is skipped when an earlier [Architect plan is still open](#one-architect-plan-at-a-time). With `launch.pull = true` in the [User config](#user-config), it then fast-forwards your checkout of the Base branch, as a Run does, and so only when the Base branch is the branch checked out: with `base ` from a clone on another branch, your checkout is left alone. 2. Creates a git worktree next to your clone, named `-architect`, detached at the head of the Base branch on `origin`, with no **Issue branch**. Your checkout, its uncommitted changes and its untracked files are never touched or scanned. 3. Runs the **Architecture review** there: a headless Claude Code session with the **Factory skills** loaded, started with the [Architecture review prompt](prompts/architecture-review.md). It looks for deepening opportunities, skips any an open issue already covers, and takes the top recommendation. If it is Strong, the review publishes it as the plan, labelled `needs-triage`: a **Spec** with **Tickets**, or a single Ticket when one session is enough. It may edit files in the worktree to check an idea, but commits and pushes nothing. It ends its final message with one line naming the plan: `Architecture review plan: `. thirdshift reads only that line. With [no Strong candidate](#no-strong-candidate) the line names another issue, and steps 5 to 7 are skipped. 4. Removes the worktree and the temporary plugin directory once the session ends, whatever the outcome. @@ -471,7 +475,7 @@ A review session that fails or is interrupted, a final message without one of th An Architect run asked for a [Run notification](#run-notifications), by `email` or by `email.always = true` in the [User config](#user-config) without `no-email`, sends exactly one, whatever its outcome, skipped included, [either](#one-at-a-time) [way](#one-architect-plan-at-a-time), and with or without `--plan-only`. It makes a Run's checks before any other work, an address and a Resend API key, and stops with exit `1` if either is missing. The email goes after the outcome is final and printed, and a failed send is only a `warning:` line on stderr: it changes neither the exit code nor stdout. - **Subject**: `[thirdshift] / Architect run: `. With a dispatched run, the outcome is that run's: `ready for review`, `merged`, `failed` or `interrupted`. Without one, it is the review's: `plan published` (with `--plan-only`), `idea filed`, `idea already filed`, `review failed` (also for a plan that fails a check) or `interrupted`. For a skipped run, it is `skipped`. The repository is left out if `origin` doesn't name one on GitHub. -- **Body**, plain text: a `Review:` line saying how the Architecture review ended, with the URL of the plan or idea issue it named (`plan published: `, `idea filed: `, `idea already filed: `, `failed` or `interrupted`); when the plan was dispatched, a `Dispatched:` line with that run's outcome; then what a Run's notification holds, for the dispatched run or else the failed review: the pull request URL (if any), the failure cause (if failed), with the [lines after it](#an-inherited-failure-links-the-base-branchs-checks) of a dispatched run that failed on an Inherited failure, a `Base fix:` line for a dispatched run that started or waited on a [Base fix](#base-fix), the session log path (if any), the hostname and how long the Architect run took. After a dispatched Spec run, it ends with a line per Ticket, as a Spec run's notification does. A skipped run's body has a `Skipped:` line giving the reason in place of the `Review:` line, then the hostname and how long it took. The reason is `an Architect run is already running on /`, or each open Architect plan with the command that picks it up: `Architect plan # "" is still open: pick it up with thirdshift <plan URL>`. +- **Body**, plain text: a `Review:` line saying how the Architecture review ended, with the URL of the plan or idea issue it named (`plan published: <url>`, `idea filed: <url>`, `idea already filed: <url>`, `failed` or `interrupted`); when the plan was dispatched, a `Dispatched:` line with that run's outcome; then what a Run's notification holds, for the dispatched run or else the failed review: the pull request URL (if any), the failure cause (if failed), with the [lines after it](#an-inherited-failure-links-the-base-branchs-checks) of a dispatched run that failed on an Inherited failure, a `Base fix:` line for a dispatched run that started or waited on a [Base fix](#base-fix), the session log path (if any), the hostname and how long the Architect run took. After a dispatched Spec run, it ends with a line per Ticket, as a Spec run's notification does. A skipped run's body has a `Skipped:` line giving the reason in place of the `Review:` line, then the hostname and how long it took. The reason is `an Architect run or a Pickup run is already running on <owner>/<repo>`, or each open Architect plan with the command that picks it up: `Architect plan #<n> "<title>" is still open: pick it up with thirdshift <plan URL>`. ### No Strong candidate @@ -482,13 +486,13 @@ Only a Strong top recommendation becomes a plan. When the review's top recommend ### One at a time -Only one Architect run per repository runs at a time on a machine. A `thirdshift architect` started while another on the same repository is still running, the Spec run or Run it dispatched included, is **skipped**: it prints `an Architect run is already running on <owner>/<repo>` as a progress line on stderr, prints nothing on stdout, and exits `0`. Skipped is not a failure. A skipped run does nothing else: no launch pull, no worktree, no agent session, and no issue filed or labelled. This holds for every Architect run, however it was started, so two can never plan the same refactor. +Only one Architect run or [Pickup run](#pickup-runs) per repository runs at a time on a machine. A `thirdshift architect` started while another on the same repository, or a `thirdshift pickup`, is still running, the Spec run or Run it dispatched included, is **skipped**: it prints `an Architect run or a Pickup run is already running on <owner>/<repo>` as a progress line on stderr, prints nothing on stdout, and exits `0`. Skipped is not a failure. A skipped run does nothing else: no launch pull, no worktree, no agent session, and no issue filed or labelled. This holds for every Architect run, however it was started, so two can never plan the same refactor. A Pickup run is skipped the same way while an Architect run or another Pickup run is still running, so the two never build one repository at once. -The repository is the one `origin` names, so two clones of one repository count as the same, and Architect runs on different repositories run at the same time. A Run or a Spec run started on an **Issue URL** is never skipped this way, and never makes an Architect run skip. +The repository is the one `origin` names, so two clones of one repository count as the same, and Architect runs on different repositories run at the same time. A Run or a Spec run started on an **Issue URL** is never skipped this way, and never makes an Architect run or a Pickup run skip. -thirdshift holds the rule with an operating-system lock on a file under `~/.thirdshift/architect-locks/`, which the Architect run's process takes without waiting, after its checks in step 1, and holds until it exits. The operating system releases the lock when that process ends for any reason, a crash, a kill or a reboot included, so there is never a stale lock to clear: the next Architect run on the repository runs normally. The file itself stays, and means nothing on its own. A review worktree that a killed Architect run left behind is removed by the next one. +thirdshift holds the rule with an operating-system lock on a file under `~/.thirdshift/architect-locks/`, one for both kinds of run, which the run's process takes without waiting, after its checks in step 1, and holds until it exits. The operating system releases the lock when that process ends for any reason, a crash, a kill or a reboot included, so there is never a stale lock to clear: the next Architect run or Pickup run on the repository runs normally. The file itself stays, and means nothing on its own. A review worktree that a killed Architect run left behind is removed by the next one. -A skipped run asked for a [Run notification](#one-run-notification) still sends its one: the notification's checks are made before the lock is tried, so a skip is always notified. +A skipped Architect run asked for a [Run notification](#one-run-notification) still sends its one: the notification's checks are made before the lock is tried, so a skip is always notified. A skipped Pickup run sends none. ### One Architect plan at a time @@ -530,10 +534,43 @@ The command's flags and the User config decide what a scheduled Architect run do The first leaves the pull request for you to review, and the second stops at the Architect plan, for you to read and run with `thirdshift <Issue URL>`. -A scheduled Architect run is skipped when another on the repository is [still running](#one-at-a-time), the Spec run or Run it dispatched included, or when an [Architect plan is still open](#one-architect-plan-at-a-time): no new refactor is planned until last night's Architect plan is closed or loses its `architect-plan` label. Merging its pull request closes it; if its run failed, pick it up with `thirdshift <plan URL>` first. Closing the pull request alone leaves the Architect plan open. A skip exits `0`, so the scheduler sees no failure, and the log has the line that says why. With Run notifications on, by `email.always = true` in the User config or `email` on the line, every night sends one email, a skipped night included, so a night without one means something is wrong: look in the log file. +A scheduled Architect run is skipped when another on the repository, or a Pickup run, is [still running](#one-at-a-time), the Spec run or Run it dispatched included, or when an [Architect plan is still open](#one-architect-plan-at-a-time): no new refactor is planned until last night's Architect plan is closed or loses its `architect-plan` label. Merging its pull request closes it; if its run failed, pick it up with `thirdshift <plan URL>` first. Closing the pull request alone leaves the Architect plan open. A skip exits `0`, so the scheduler sees no failure, and the log has the line that says why. With Run notifications on, by `email.always = true` in the User config or `email` on the line, every night sends one email, a skipped night included, so a night without one means something is wrong: look in the log file. Any scheduler that runs the command works; a systemd user timer needs lingering on (`loginctl enable-linger`) to fire while you are logged out. +## Pickup runs + +`thirdshift pickup` starts a **Pickup run**: one pass, with no **Issue URL**, that takes the lowest-numbered **Ready issue** in the repository and runs it, so that an issue you have already marked ready needs no command typed for it. Start it from a clone of the repository, on the **Base branch**, or name the Base branch with `base <branch>`: + +```sh +cd ~/repos/widgets +thirdshift pickup # take the lowest-numbered Ready issue and run it +thirdshift pickup merge parallel 2 # merge its pull request; two Tickets at once if it is a Spec +thirdshift pickup base main no-merge # run it on main, whatever branch the clone is on, and leave its pull request for review +``` + +A Ready issue is an open issue that: + +- is labelled `ready-for-agent`: an issue without the label is never taken; +- has none of the labels that make an **Unready Ticket**, `ready-for-human`, `needs-info`, `wontfix` and `needs-triage`, so a contradictory label errs on the side of not running; +- carries no [Claim](#the-claim): it is not labelled `in-progress`; +- was never started: no **Issue branch** for it is on `origin`, and no pull request from one exists, open, merged or closed. So a Pickup run never does a [Continuation](#continuation), and an issue whose Run failed waits for you. + +A Pickup run: + +1. Makes the checks an [Architect run](#architect-runs) makes, before anything else: `origin` is a GitHub repository, git has a `user.name` and `user.email`, HEAD is not detached unless `base <branch>` names the Base branch, and the Base branch exists on `origin` with your local copy not ahead of it. A check that fails stops the pass with exit `1`, before any label is read or changed. +2. Tries the lock an Architect run takes, and is [skipped](#one-at-a-time) if an Architect run or another Pickup run on the repository is still running on the machine. +3. Lists the repository's open issues labelled `ready-for-agent`, lowest number first, and takes the first that is a Ready issue, saying so on stderr: `taking Ready issue #<n> "<title>", as thirdshift <Issue URL> would`. One issue a pass: the rest wait for the next. +4. Dispatches it exactly as `thirdshift <Issue URL>` would from the same clone on the Pickup run's Base branch: a [Spec run](#spec-runs) when the issue has sub-issues, a Run otherwise. That run makes the Claim, so the issue's `ready-for-agent` is swapped for `in-progress` and no later pass takes it again. + +`merge`, `no-merge`, `base-fix`, `no-base-fix`, `parallel <n>`, `email`, optionally followed by an address, and `no-email`, with or without dashes, are for the dispatched run, and mean what they do for `thirdshift <Issue URL>`. The [User config](#user-config) sets what they leave unsaid: `merge.always`, `base.fix`, `spec.parallel`, `email.always`, `launch.pull` and `logs.dir`. `parallel <n>` applies when the Ready issue is a Spec and is ignored, with no error, when it isn't, unlike on an Issue URL: the command can't know which it will take. `base <branch>` (or `--base <branch>`) names the Base branch as it does for [`architect`](#architect-runs), and the dispatched run takes it: its Issue branch or Spec branch is branched off `<branch>`, and its pull request targets it. + +`pickup` is a command only as the first argument, and takes nothing but those flags, each at most once: a focus, `--plan-only`, an Issue URL, a repeated or contradictory flag, or any other argument is an argument error (exit `2`). + +The dispatched run's ending is the Pickup run's: its exit code, its pull request's URL alone on stdout, and its last line on stderr, `PR <url> is ready for review` or `PR <url> is merged`. If it fails, the Pickup run fails as that [Failed run](#failed-runs) or Failed spec run does. After an [Inherited failure](#an-inherited-failure-links-the-base-branchs-checks), the command it offers is `thirdshift <Issue URL>` with the dispatched run's flags and `base-fix`, since another Pickup run would not take a started issue again. + +A pass is **skipped** when the lock is held, or when the repository has no Ready issue: it exits `0` with stdout empty and one line of reason on stderr, `an Architect run or a Pickup run is already running on <owner>/<repo>` or `no Ready issue on <owner>/<repo>`. Skipped is not a failure. A skipped pass starts no agent session, changes no label, and sends no Run notification. + ## Building from source Building needs the Rust toolchain. From a clone of this repository: diff --git a/src/architect.rs b/src/architect.rs index 91417a1a4..06e2b6dc5 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -6,26 +6,24 @@ //! dispatch. A review that found no Strong candidate published no plan, and //! the Architect run ends on the issue it named instead: the idea issue it //! filed for its top recommendation, or the open issue that already covers -//! it. Only one Architect run per repository runs at a time on a machine: one -//! started while another is still running is skipped, before any review. So -//! is one that finds an Architect plan still open on the repository: it never -//! retries or dispatches an Architect plan that is already there. +//! it. Only one Architect run or Pickup run per repository runs at a time on +//! a machine: an Architect run started while another of either is still +//! running is skipped, before any review. So is one that finds an Architect +//! plan still open on the repository: it never retries or dispatches an +//! Architect plan that is already there. use std::fmt; -use std::fs::{self, File, TryLockError}; use std::path::{Path, PathBuf}; -use anyhow::{Context, Result, anyhow, bail}; +use anyhow::{Result, anyhow, bail}; use chrono::{DateTime, Utc}; -use crate::config; use crate::failed_run::FailedRun; -use crate::git::Git; use crate::github; use crate::interrupt; -use crate::issue::{IssueUrl, Repo}; +use crate::issue::IssueUrl; +use crate::launch::{self, AlreadyRunning, Launch, Start}; use crate::plugin::Plugin; -use crate::preflight; use crate::progress; use crate::prompt; use crate::run; @@ -69,7 +67,7 @@ impl Outcome { /// The URLs of the issues the Architect run ended on, as its stdout /// carries them when nothing was dispatched: the issue its Architecture /// review ended on, or each open Architect plan it was skipped for. None - /// when it was skipped as another is still running. + /// when it was skipped as another, or a Pickup run, is still running. pub fn urls(&self) -> Vec<&str> { match self { Self::Skipped(Skipped::AlreadyRunning(_)) => Vec::new(), @@ -94,9 +92,9 @@ impl fmt::Display for Outcome { /// skipped run's progress line and its Run notification give it. #[derive(Debug)] pub enum Skipped { - /// Another Architect run on this repository is still running on this - /// machine, the Spec run or Run it dispatched included. - AlreadyRunning(Repo), + /// Another Architect run on this repository, or a Pickup run, is still + /// running on this machine, the Spec run or Run it dispatched included. + AlreadyRunning(AlreadyRunning), /// These Architect plans, each with its title, are still open on this /// repository: at least one. OpenPlans(Vec<(IssueUrl, String)>), @@ -105,9 +103,7 @@ pub enum Skipped { impl fmt::Display for Skipped { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::AlreadyRunning(repo) => { - write!(f, "an Architect run is already running on {}", repo.slug()) - } + Self::AlreadyRunning(running) => running.fmt(f), Self::OpenPlans(plans) => { let still_open: Vec<String> = plans .iter() @@ -190,9 +186,8 @@ impl fmt::Display for Reviewed { /// after the plan is published leaves its labels as the review left them. /// /// Once the preflight checks pass, and before anything else, the Architect -/// run is skipped, with nothing done, if another on the same repository is -/// still running on this machine. Otherwise this process is that repository's -/// one Architect run until it exits, through whatever it dispatches. It is +/// run is skipped, with nothing done, if another on the same repository, or a +/// Pickup run, is still running on this machine: see [`launch::start`]. It is /// then skipped, likewise, if the repository has an open Architect plan: only /// then, so that the plan of an Architect run still running is never taken /// for an unfinished one. @@ -204,29 +199,22 @@ pub fn run( ) -> Result<Outcome, FailedRun> { let started = Utc::now(); let timestamp = started.format("%Y%m%dT%H%M%SZ").to_string(); - let (launch, origin, repo) = launch()?; - preflight::check_identity(&launch)?; - let checked_out = launch - .run(&["symbolic-ref", "--quiet", "--short", "HEAD"]) - .ok(); - let base = base - .or(checked_out.as_deref()) - .context( - "HEAD is detached; check out the branch the Architecture review should scan, \ - or name it with base <branch>", - )? - .to_string(); - preflight::check_base_branch(&launch, &base)?; - let Some(lock) = try_run_lock(&repo)? else { - return Ok(Outcome::Skipped(Skipped::AlreadyRunning(repo))); + let Launch { + git: launch, + origin, + repo, + checked_out, + base, + } = match launch::start(base)? { + Start::Clear(launch) => launch, + Start::AlreadyRunning(running) => { + return Ok(Outcome::Skipped(Skipped::AlreadyRunning(running))); + } }; - // Never closed, so the lock is held for as long as this process lives, - // through the Spec run or Run its plan is dispatched as, and the - // operating system releases it however the process ends. - std::mem::forget(lock); let open_plans = github::open_issues_labelled(&repo.slug(), ARCHITECT_PLAN_LABEL)?; if !open_plans.is_empty() { - return Ok(Outcome::Skipped(Skipped::OpenPlans(open_plans))); + let open_plans = open_plans.into_iter().map(|plan| (plan.issue, plan.title)); + return Ok(Outcome::Skipped(Skipped::OpenPlans(open_plans.collect()))); } if launch_pull { run::pull_base_branch(&launch, checked_out.as_deref(), &base); @@ -247,44 +235,6 @@ pub fn run( }) } -/// The Launch directory, the URL of its `origin`, and the GitHub repository -/// that names: an Architect run's, as it has no Issue URL to name one. -fn launch() -> Result<(Git, String, Repo)> { - let launch = Git::new(std::env::current_dir().context("no current directory")?); - let origin = launch.run(&["config", "remote.origin.url"])?; - let repo = Repo::of_origin(&origin) - .with_context(|| format!("origin {origin} is not a GitHub repository"))?; - Ok((launch, origin, repo)) -} - -/// The repository an Architect run started from the Launch directory is on. -pub fn repo() -> Result<Repo> { - launch().map(|(_, _, repo)| repo) -} - -/// Try, without waiting, for the lock that makes its holder the one Architect -/// run on `repo` on this machine: an advisory lock on a file under -/// `~/.thirdshift` named for the repository, as GitHub compares names, -/// whatever their case. It is held until the file returned is closed, or the -/// process ends. `None` if another process holds it: an Architect run on -/// `repo` is still running. The lock file itself is never deleted, and means -/// nothing unless locked. -fn try_run_lock(repo: &Repo) -> Result<Option<File>> { - let dir = config::home()? - .join(".thirdshift/architect-locks") - .join(repo.owner.to_ascii_lowercase()); - fs::create_dir_all(&dir).with_context(|| format!("can't create {}", dir.display()))?; - let path = dir.join(format!("{}.lock", repo.name.to_ascii_lowercase())); - let file = File::create(&path).with_context(|| format!("can't open {}", path.display()))?; - match file.try_lock() { - Ok(()) => Ok(Some(file)), - Err(TryLockError::WouldBlock) => Ok(None), - Err(TryLockError::Error(error)) => { - Err(error).with_context(|| format!("can't lock {}", path.display())) - } - } -} - /// The Architecture review session in `worktree`, which is removed once the /// session ends. Returns the session's final message. `log` is left at the /// most recent session's log. diff --git a/src/args.rs b/src/args.rs index f76af0aef..4dcfc7eca 100644 --- a/src/args.rs +++ b/src/args.rs @@ -1,5 +1,5 @@ //! The command line: which command, and for a Run, its Issue URL and flags, -//! or for an Architect run, its focus and flags. +//! for an Architect run, its focus and flags, or for a Pickup run, its flags. use std::iter::Peekable; use std::mem::discriminant; @@ -22,6 +22,7 @@ pub enum Command { /// `email-test`, with the address it was given, if any. EmailTest(Option<String>), Architect(ArchitectArgs), + Pickup(PickupArgs), Run(RunArgs), } @@ -46,8 +47,21 @@ pub struct ArchitectArgs { pub dispatch: Option<DispatchArgs>, } -/// What an Architect run's flags ask of the Spec run or Run it dispatches -/// its plan as, each meaning what it does in [`RunArgs`]. +/// A Pickup run's arguments. +#[derive(Debug, PartialEq, Eq)] +pub struct PickupArgs { + /// The Base branch `base <branch>` named, if given; without it, the + /// branch checked out in the Launch directory is the Base branch. + pub base: Option<String>, + /// What `email` or `no-email` asked for, if either was given; without + /// one, the User config decides. + pub email: Option<NotificationAsk>, + /// The flags for the Spec run or Run the Ready issue is dispatched as. + pub dispatch: DispatchArgs, +} + +/// What the flags of an Architect run or a Pickup run ask of the Spec run or +/// Run it dispatches, each meaning what it does in [`RunArgs`]. #[derive(Debug, Default, PartialEq, Eq)] pub struct DispatchArgs { pub goal: Option<Goal>, @@ -94,8 +108,8 @@ pub struct RunArgs { } /// Parse the arguments after the program name. `help`, `version`, `update`, -/// `setup`, `email-test` and `architect` are commands only as the first -/// argument. Otherwise it is a Run: one Issue URL, with each Run flag at +/// `setup`, `email-test`, `architect` and `pickup` are commands only as the +/// first argument. Otherwise it is a Run: one Issue URL, with each Run flag at /// most once, before or after it. /// `email` may be followed by the address to send the Run notification to, /// and `parallel` must be followed by a whole number from 1 up. @@ -120,6 +134,7 @@ pub fn parse(args: &[String]) -> Result<Command> { }; } Some("architect") => return parse_architect(&args[1..]).map(Command::Architect), + Some("pickup") => return parse_pickup(&args[1..]).map(Command::Pickup), _ => {} } let mut issue = None; @@ -228,11 +243,7 @@ fn parse_architect(args: &[String]) -> Result<ArchitectArgs> { _ => focus = Some(arg.clone()), } } - let dispatch = DispatchArgs { - goal: flags.goal, - parallel: flags.parallel, - base_fix: flags.base_fix, - }; + let (email, dispatch) = flags.for_dispatch(); if plan_only && dispatch != DispatchArgs::default() { bail!( "merge, no-merge, parallel, base-fix and no-base-fix can't be used with \ @@ -242,13 +253,36 @@ fn parse_architect(args: &[String]) -> Result<ArchitectArgs> { Ok(ArchitectArgs { focus, base, - email: flags.email, + email, dispatch: (!plan_only).then_some(dispatch), }) } -/// The flags a Run and an Architect run both take, as given so far: each -/// field is what [`RunArgs`] says of the one it becomes. +/// Parse the arguments after `pickup`: its flags, each at most once, in any +/// order, and nothing else. `base` must be followed by the Base branch. +fn parse_pickup(args: &[String]) -> Result<PickupArgs> { + let mut base = None; + let mut flags = RunFlags::default(); + let mut args = args.iter().peekable(); + while let Some(arg) = args.next() { + if flags.take(arg, &mut args)? { + continue; + } + match arg.as_str() { + "base" | "--base" => ask_base(&mut base, arg, args.next())?, + _ => bail!("unexpected argument after pickup: {arg}"), + } + } + let (email, dispatch) = flags.for_dispatch(); + Ok(PickupArgs { + base, + email, + dispatch, + }) +} + +/// The flags a Run, an Architect run and a Pickup run all take, as given so +/// far: each field is what [`RunArgs`] says of the one it becomes. #[derive(Default)] struct RunFlags { goal: Option<Goal>, @@ -258,6 +292,18 @@ struct RunFlags { } impl RunFlags { + /// The flags as an Architect run or a Pickup run takes them: what was + /// asked about its Run notification, and the flags for the run it + /// dispatches. + fn for_dispatch(self) -> (Option<NotificationAsk>, DispatchArgs) { + let dispatch = DispatchArgs { + goal: self.goal, + parallel: self.parallel, + base_fix: self.base_fix, + }; + (self.email, dispatch) + } + /// Record what `arg` asks for, if it is one of these flags, with or /// without its dashes, taking from `rest` the address after `email`, if /// one is there, and the number after `parallel`. False, taking nothing, @@ -720,6 +766,176 @@ mod tests { } } + /// The Pickup run `args` parse to. + fn pickup_args(args: &[&str]) -> PickupArgs { + match parse_strs(args) { + Ok(Command::Pickup(pickup_args)) => pickup_args, + Ok(_) => panic!("{args:?}: not a Pickup run"), + Err(error) => panic!("{args:?}: {error:#}"), + } + } + + #[test] + fn pickup_takes_each_of_its_flags_with_or_without_dashes_in_any_order() { + let none = PickupArgs { + base: None, + email: None, + dispatch: DispatchArgs::default(), + }; + assert_eq!(pickup_args(&["pickup"]), none); + let all = PickupArgs { + base: Some("develop".to_string()), + email: Some(NotificationAsk::Send(Some("me@example.com".to_string()))), + dispatch: DispatchArgs { + goal: Some(Goal::Merged), + parallel: NonZeroUsize::new(2), + base_fix: Some(BaseFixAsk::Allow), + }, + }; + for args in [ + vec![ + "pickup", + "merge", + "parallel", + "2", + "base-fix", + "email", + "me@example.com", + "base", + "develop", + ], + vec![ + "pickup", + "--base", + "develop", + "--email", + "me@example.com", + "--base-fix", + "--parallel", + "2", + "--merge", + ], + ] { + assert_eq!(pickup_args(&args), all, "{args:?}"); + } + let cautious = PickupArgs { + base: None, + email: Some(NotificationAsk::Skip), + dispatch: DispatchArgs { + goal: Some(Goal::ReadyForReview), + parallel: None, + base_fix: Some(BaseFixAsk::Forbid), + }, + }; + for args in [ + ["pickup", "no-merge", "no-base-fix", "no-email"], + ["pickup", "--no-email", "--no-merge", "--no-base-fix"], + ] { + assert_eq!(pickup_args(&args), cautious, "{args:?}"); + } + let bare_email = pickup_args(&["pickup", "email", "merge"]); + assert_eq!(bare_email.email, Some(NotificationAsk::Send(None))); + assert_eq!(bare_email.dispatch.goal, Some(Goal::Merged)); + } + + #[test] + fn pickup_rejects_a_focus_plan_only_an_issue_url_and_any_other_argument() { + for (args, error) in [ + ( + vec!["pickup", "the Spec run"], + "unexpected argument after pickup: the Spec run", + ), + ( + vec!["pickup", "merge", "--plan-only"], + "unexpected argument after pickup: --plan-only", + ), + ( + vec!["pickup", URL], + "unexpected argument after pickup: https://github.com/acme/widgets/issues/7", + ), + ( + vec!["pickup", "--verbose"], + "unexpected argument after pickup: --verbose", + ), + ( + vec!["pickup", "--spec-branch", "issue-7"], + "unexpected argument after pickup: --spec-branch", + ), + ( + vec!["pickup", "pickup"], + "unexpected argument after pickup: pickup", + ), + ] { + assert_eq!(rejection(&args), error, "{args:?}"); + } + } + + #[test] + fn pickup_rejects_repeated_contradictory_and_malformed_flags() { + for (args, error) in [ + ( + vec!["pickup", "merge", "--no-merge"], + "merge and no-merge can't be used together", + ), + ( + vec!["pickup", "no-merge", "no-merge"], + "repeated argument: no-merge", + ), + ( + vec!["pickup", "base-fix", "no-base-fix"], + "base-fix and no-base-fix can't be used together", + ), + ( + vec!["pickup", "--base-fix", "base-fix"], + "repeated argument: base-fix", + ), + ( + vec!["pickup", "no-email", "email", "me@example.com"], + "email and no-email can't be used together", + ), + ( + vec!["pickup", "email", "--email"], + "repeated argument: --email", + ), + ( + vec!["pickup", "parallel", "2", "parallel", "3"], + "repeated argument: parallel", + ), + ( + vec!["pickup", "parallel"], + "parallel must be followed by a whole number from 1 up", + ), + ( + vec!["pickup", "parallel", "merge"], + "parallel must be followed by a whole number from 1 up, not merge", + ), + (vec!["pickup", "base"], "base must be followed by a branch"), + ( + vec!["pickup", "--base", "--merge"], + "--base must be followed by a branch, not --merge", + ), + ( + vec!["pickup", "base", "develop", "base", "main"], + "repeated argument: base", + ), + ] { + assert_eq!(rejection(&args), error, "{args:?}"); + } + } + + #[test] + fn pickup_is_a_command_only_as_the_first_argument() { + for (args, error) in [ + ( + vec![URL, "pickup"], + "unexpected argument after the Issue URL: pickup", + ), + (vec!["merge", "pickup"], "not a GitHub issue URL: pickup"), + ] { + assert_eq!(rejection(&args), error, "{args:?}"); + } + } + #[test] fn a_bare_flag_asks_for_email_to_and_an_address_after_the_flag_is_the_one_asked_for() { let to_flag_address = || NotificationAsk::Send(Some("flag@example.com".to_string())); diff --git a/src/base_fix.rs b/src/base_fix.rs index 56ab47641..d64c070ed 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -477,13 +477,14 @@ fn open_fix_covering( let before_checks = issue_title(base, ""); let covering = github::open_issues_labelled(&issue.repo_slug(), BASE_FIX_LABEL)? .into_iter() - .filter(|(open, title)| { - open.number != issue.number - && title + .filter(|open| { + open.issue.number != issue.number + && open + .title .strip_prefix(&before_checks) .is_some_and(|checks| inherited.iter().all(|check| names(checks, &check.name))) }) - .map(|(open, _)| open) + .map(|open| open.issue) .min_by_key(|open| open.number); Ok(covering) } diff --git a/src/branch.rs b/src/branch.rs index 5617624d1..e7566379e 100644 --- a/src/branch.rs +++ b/src/branch.rs @@ -57,32 +57,13 @@ impl Selection { } } -/// 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`): the -/// highest number seen on either decides. Fails if a local copy of the chosen branch in the launch repository differs from +/// Pick the Issue branch for `issue` from the Issue branches on origin and +/// their PRs in any state: the highest number seen on either decides. Fails +/// if a local copy of the chosen branch in the Launch directory differs from /// origin's: the Run replaces it and deletes it at cleanup. pub fn select(launch: &Git, issue: &IssueUrl) -> Result<Selection> { 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 Used { on_origin, prs } = used(launch, issue)?; let highest = on_origin .iter() @@ -119,6 +100,48 @@ pub fn select(launch: &Git, issue: &IssueUrl) -> Result<Selection> { } } +/// Whether `issue` was ever started: an Issue branch for it is on origin, or +/// a pull request from one exists, open, merged or closed. +pub fn started(launch: &Git, issue: &IssueUrl) -> Result<bool> { + let Used { on_origin, prs } = used(launch, issue)?; + Ok(!on_origin.is_empty() || !prs.is_empty()) +} + +/// What has been used of an issue's Issue branches. +struct Used { + /// Those on origin, each by its number, with its head. + on_origin: Vec<(u64, String)>, + /// The pull requests from them, in any state, each with its Issue + /// branch's number. + prs: Vec<(u64, PullRequest)>, +} + +/// What has been used of `issue`'s Issue branches: those on origin (one `git +/// ls-remote`) and the pull requests from them (one `gh pr list`). +fn used(launch: &Git, issue: &IssueUrl) -> Result<Used> { + 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 = 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 = github::pull_requests_with_head_prefix(issue, &first_branch)? + .into_iter() + .filter_map(|pr| Some((branch_number(issue, &pr.head)?, pr))) + .collect(); + Ok(Used { on_origin, prs }) +} + /// 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<()> { diff --git a/src/claim.rs b/src/claim.rs index e1c537cc0..26e56587b 100644 --- a/src/claim.rs +++ b/src/claim.rs @@ -16,6 +16,18 @@ const IN_PROGRESS: &str = "in-progress"; /// if the repository lacks it. const IN_PROGRESS_DESCRIPTION: &str = "A Claim: thirdshift has taken this issue"; +/// Whether an issue with `labels` carries a Claim, whatever else it is +/// labelled. +pub fn is_on(labels: &[String]) -> bool { + has(labels, IN_PROGRESS) +} + +/// Whether `label` is one of `labels`, whatever its case: GitHub's label +/// names are case-insensitive. +fn has(labels: &[String], label: &str) -> bool { + labels.iter().any(|name| name.eq_ignore_ascii_case(label)) +} + /// 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 @@ -30,9 +42,7 @@ pub fn make(issue: &IssueUrl) -> Result<()> { /// [`make`], its failure as `gh` gave it. 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) = (is_on(&labels), has(&labels, READY_FOR_AGENT)); if claimed && !ready { return Ok(()); } diff --git a/src/github.rs b/src/github.rs index 70a7279eb..3492a91ef 100644 --- a/src/github.rs +++ b/src/github.rs @@ -219,11 +219,17 @@ pub fn close_issue(issue: &IssueUrl, comment: &str) -> Result<()> { ]) } +/// An open issue as a listing of its repository's issues gives it. +pub struct ListedIssue { + pub issue: IssueUrl, + pub title: String, + pub labels: Vec<String>, +} + /// Every open issue labelled `label` in the repository `repo`, an -/// `owner/repo`, with its title, newest first. They come from GitHub's issue -/// list, not its search, whose index can be a while behind an issue just -/// opened. -pub fn open_issues_labelled(repo: &str, label: &str) -> Result<Vec<(IssueUrl, String)>> { +/// `owner/repo`, newest first. They come from GitHub's issue list, not its +/// search, whose index can be a while behind an issue just opened. +pub fn open_issues_labelled(repo: &str, label: &str) -> Result<Vec<ListedIssue>> { let json = gh_json(&[ "issue", "list", @@ -234,7 +240,7 @@ pub fn open_issues_labelled(repo: &str, label: &str) -> Result<Vec<(IssueUrl, St "--state", "open", "--json", - "url,title", + "url,title,labels", "--limit", "1000", ])?; @@ -247,7 +253,11 @@ pub fn open_issues_labelled(repo: &str, label: &str) -> Result<Vec<(IssueUrl, St .as_str() .with_context(|| format!("gh issue list output has no {name}")) }; - Ok((IssueUrl::parse(field("url")?)?, field("title")?.to_string())) + Ok(ListedIssue { + issue: IssueUrl::parse(field("url")?)?, + title: field("title")?.to_string(), + labels: label_names(listed)?, + }) }) .collect() } diff --git a/src/launch.rs b/src/launch.rs new file mode 100644 index 000000000..07adf44c4 --- /dev/null +++ b/src/launch.rs @@ -0,0 +1,134 @@ +//! The start of a run with no Issue URL, an Architect run or a Pickup run: +//! its repository is the one the Launch directory's `origin` names, its Base +//! branch the one its command named or else the one checked out there. Both +//! make the same preflight checks, then try the same lock, so only one of +//! them per repository runs at a time on a machine. + +use std::fmt; +use std::fs::{self, File, TryLockError}; + +use anyhow::{Context, Result}; + +use crate::config; +use crate::git::Git; +use crate::issue::Repo; +use crate::preflight; + +/// The Launch directory of a run with no Issue URL, its checks passed and +/// its repository's lock held. +pub struct Launch { + pub git: Git, + /// The URL of its `origin`. + pub origin: String, + /// The GitHub repository `origin` names. + pub repo: Repo, + /// The branch checked out there, or none on a detached HEAD. + pub checked_out: Option<String>, + /// The run's Base branch. + pub base: String, +} + +/// How a run with no Issue URL starts. +pub enum Start { + /// Its checks passed and its repository's lock is its own. + Clear(Launch), + /// It is skipped, with nothing done. + AlreadyRunning(AlreadyRunning), +} + +/// Why a run with no Issue URL is skipped at its start: another on the +/// repository, an Architect run or a Pickup run, is still running on this +/// machine, the Spec run or Run it dispatched included. Its `Display` is the +/// reason, as the skipped run gives it. +#[derive(Debug)] +pub struct AlreadyRunning(pub Repo); + +impl fmt::Display for AlreadyRunning { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!( + f, + "an Architect run or a Pickup run is already running on {}", + self.0.slug() + ) + } +} + +/// Start a run with no Issue URL from the Launch directory, the current +/// directory. Its Base branch is `base`, the branch its command named, +/// whatever the Launch directory has checked out, or without one the branch +/// checked out there, so a detached HEAD with neither fails it. +/// +/// Once the preflight checks pass, the run is [`AlreadyRunning`], with +/// nothing done, if another on the same repository is still running on this +/// machine. Otherwise this process is that repository's one such run until it +/// exits, through whatever it dispatches. +pub fn start(base: Option<&str>) -> Result<Start> { + let (git, origin, repo) = directory()?; + preflight::check_identity(&git)?; + let checked_out = git + .run(&["symbolic-ref", "--quiet", "--short", "HEAD"]) + .ok(); + let base = base + .or(checked_out.as_deref()) + .context( + "HEAD is detached; check out the branch the work should be based on, \ + or name it with base <branch>", + )? + .to_string(); + preflight::check_base_branch(&git, &base)?; + let Some(lock) = try_run_lock(&repo)? else { + return Ok(Start::AlreadyRunning(AlreadyRunning(repo))); + }; + // Never closed, so the lock is held for as long as this process lives, + // through the Spec run or Run it dispatches, and the operating system + // releases it however the process ends. + std::mem::forget(lock); + Ok(Start::Clear(Launch { + git, + origin, + repo, + checked_out, + base, + })) +} + +/// The Launch directory, the URL of its `origin`, and the GitHub repository +/// that names. +fn directory() -> Result<(Git, String, Repo)> { + let git = Git::new(std::env::current_dir().context("no current directory")?); + let origin = git.run(&["config", "remote.origin.url"])?; + let repo = Repo::of_origin(&origin) + .with_context(|| format!("origin {origin} is not a GitHub repository"))?; + Ok((git, origin, repo)) +} + +/// The repository a run with no Issue URL started from the Launch directory +/// is on. +pub fn repo() -> Result<Repo> { + directory().map(|(_, _, repo)| repo) +} + +/// Try, without waiting, for the lock that makes its holder the one Architect +/// run or Pickup run on `repo` on this machine: an advisory lock on a file +/// under `~/.thirdshift` named for the repository, as GitHub compares names, +/// whatever their case. It is held until the file returned is closed, or the +/// process ends. `None` if another process holds it: one of them on `repo` is +/// still running. The lock file itself is never deleted, and means nothing +/// unless locked. Its directory keeps the name it had when only an Architect +/// run took the lock, so that a thirdshift from before the Pickup run and +/// one from after still exclude each other. +fn try_run_lock(repo: &Repo) -> Result<Option<File>> { + let dir = config::home()? + .join(".thirdshift/architect-locks") + .join(repo.owner.to_ascii_lowercase()); + fs::create_dir_all(&dir).with_context(|| format!("can't create {}", dir.display()))?; + let path = dir.join(format!("{}.lock", repo.name.to_ascii_lowercase())); + let file = File::create(&path).with_context(|| format!("can't open {}", path.display()))?; + match file.try_lock() { + Ok(()) => Ok(Some(file)), + Err(TryLockError::WouldBlock) => Ok(None), + Err(TryLockError::Error(error)) => { + Err(error).with_context(|| format!("can't lock {}", path.display())) + } + } +} diff --git a/src/main.rs b/src/main.rs index d887c5efe..6afdf37e8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -13,7 +13,9 @@ mod github; mod host; mod interrupt; mod issue; +mod launch; mod notification; +mod pickup; mod plugin; mod poll; mod preflight; @@ -30,13 +32,15 @@ mod update; mod worktree; use std::io::Write; +use std::num::NonZeroUsize; use std::process::ExitCode; use architect::{Outcome, Reviewed}; -use args::{ArchitectArgs, Command, RunArgs}; +use args::{ArchitectArgs, Command, DispatchArgs, PickupArgs, RunArgs}; use base_fix::{Advice, BaseFixAsk}; use config::UserConfig; use failed_run::FailedRun; +use issue::IssueUrl; use notification::{ArchitectNotification, NotificationAsk, RunNotification}; use run::{Ended, Goal, StartedBy}; use spec_run::Parallel; @@ -51,6 +55,8 @@ usage: thirdshift <Issue URL> Run the factory on the iss thirdshift architect [<focus>] Review the Base branch's architecture, publish a plan for a refactor, and run it thirdshift architect [<focus>] --plan-only Publish and mark ready the plan for a refactor, and stop there thirdshift architect base <branch> [<focus>] Do either with <branch> as the Base branch, from a clone on any branch + thirdshift pickup Take the lowest-numbered Ready issue in the repository and run it + thirdshift pickup base <branch> Do that with <branch> as the Base branch, from a clone on any branch thirdshift email-test [<address>] Send a test email through Resend, to check the email setup thirdshift setup Choose your defaults, then write the User config with every setting thirdshift update Update thirdshift to the latest release @@ -132,8 +138,8 @@ The review starts at <branch>'s head on origin, and the run the plan is dispatch off <branch> and targets it with its pull request. <branch> must exist on origin, with no local copy of it ahead, and launch.pull updates the clone only when <branch> is the branch checked out. base goes with --plan-only too, before or after the focus and the other flags. base is -for architect only: thirdshift <Issue URL> doesn't take it. Without base, the Base branch is -the branch checked out. +for architect and pickup only: thirdshift <Issue URL> doesn't take it. Without base, the Base +branch is the branch checked out. A review that finds no Strong candidate publishes no plan. It files its top recommendation as one idea issue labelled needs-triage, or names the open issue that already covers it, and @@ -144,11 +150,12 @@ A review that fails, is interrupted, or ends without naming one of these issues Architect run and leaves any plan it published labelled needs-triage. One that finds no deepening opportunity at all has no issue to name, so it fails the Architect run too. -Only one Architect run per repository runs at a time on a machine. One started while another -on the same repository is still running, the Spec run or Run it dispatched included, is -skipped: it prints an Architect run is already running on <owner>/<repo>, does nothing else -and exits 0. Nothing is left to clear once that other run ends, however it ends. Runs -started on an Issue URL are never skipped this way. +Only one Architect run or Pickup run per repository runs at a time on a machine. An Architect +run started while another on the same repository, or a Pickup run, is still running, the Spec +run or Run it dispatched included, is skipped: it prints an Architect run or a Pickup run is +already running on <owner>/<repo>, does nothing else and exits 0. Nothing is left to clear +once that other run ends, however it ends. Runs started on an Issue URL are never skipped +this way. An Architect run that finds an open issue labelled architect-plan is skipped too, before any review, with or without --plan-only: the last Architect plan is not finished. It names @@ -160,6 +167,26 @@ Architect plan, so one whose run failed stays open until you pick it up. To run an Architect run on a schedule, have the operating system's scheduler, such as cron, run thirdshift architect base main from the clone: the README's \"On a schedule\" has a crontab entry. +pickup starts a Pickup run from the clone, with no Issue URL: one pass, which takes the +lowest-numbered Ready issue in the repository and dispatches it as thirdshift <Issue URL> +would: a Spec run on a Spec, a Run otherwise. A Ready issue is an open issue labelled +ready-for-agent that has none of ready-for-human, needs-info, wontfix and needs-triage, is +not in-progress, and was never started: no Issue branch for it is on origin, and no pull +request from one exists, open, merged or closed. A line on stderr names the issue taken, and +the run it is dispatched as makes the Claim on it. The Pickup run ends as that run does, with +its exit code and its PR's URL. merge, --no-merge, base-fix, --no-base-fix, --email, +--no-email and parallel <n> apply to that run, as do the User config's defaults; parallel <n> +is ignored when the issue is not a Spec. base <branch> names the Pickup run's Base branch as +it does an Architect run's, and the dispatched run branches off <branch> and targets it: + + thirdshift pickup base main + +pickup takes nothing else: no focus and no --plan-only. + +A Pickup run is skipped, exiting 0 with nothing on stdout and one line on stderr saying why, +when the repository has no Ready issue, and while an Architect run or another Pickup run on +the same repository is still running on this machine. + --email, --email <address> and --no-email ask an Architect run for its Run notification as they do a Run, with or without --plan-only, and email.always sets the default. It sends one for the whole Architect run, however it ends: how the review ended, with the plan or idea @@ -234,6 +261,7 @@ fn main() -> ExitCode { ); } Ok(Command::Architect(architect_args)) => return architect(architect_args), + Ok(Command::Pickup(pickup_args)) => return pickup(pickup_args), Ok(Command::Run(run_args)) => run_args, Err(error) => return argument_error(format_args!("{error:#}")), }; @@ -299,10 +327,10 @@ fn main() -> ExitCode { /// issue that already covers its top recommendation. One whose review or /// plan fails puts the cause and the session log on stderr. One that is /// skipped says why on stderr, and is no failure: as another on its -/// repository is still running, it puts nothing on stdout, and as Architect -/// plans are still open there, the URL of each. If asked, by the command or -/// the User config, it sends one Run notification, however it ended, skipped -/// included; the run it dispatched sends none of its own. +/// repository, or a Pickup run, is still running, it puts nothing on stdout, +/// and as Architect plans are still open there, the URL of each. If asked, by +/// the command or the User config, it sends one Run notification, however it +/// ended, skipped included; the run it dispatched sends none of its own. fn architect(args: ArchitectArgs) -> ExitCode { let config = match user_config() { Ok(config) => config, @@ -332,24 +360,10 @@ fn architect(args: ArchitectArgs) -> ExitCode { "dispatching the plan {url}, as thirdshift {url} would", url = plan.url )); - Some(run::run_to_end( - plan, - dispatch.goal.unwrap_or(config.default_goal()), - &config.logs_dir, - config.launch_pull, - Parallel::new(dispatch.parallel, config.spec_parallel), - StartedBy::ArchitectRun { base }, - dispatch.base_fix.clone().unwrap_or_else(|| { - // The plan is retried as a Run of its own: another - // Architect run would start a new review instead. - config.default_base_fix(args::retry_with_base_fix( - plan, - dispatch.goal, - email_flag.as_ref(), - dispatch.parallel, - )) - }), - )) + // The plan is retried as a Run of its own: another Architect + // run would start a new review instead. + let (email, parallel) = (email_flag.as_ref(), dispatch.parallel); + Some(dispatched(plan, base, dispatch, email, parallel, &config)) } _ => None, }; @@ -369,6 +383,84 @@ fn architect(args: ArchitectArgs) -> ExitCode { code } +/// A Pickup run: the search for the lowest-numbered Ready issue in the Launch +/// directory's repository, then that issue dispatched as `thirdshift <Issue +/// URL>` with the same flags would be, but on the Pickup run's Base branch, +/// whatever the Launch directory has checked out. The dispatched run's ending +/// is the Pickup run's, its Run notification included. +/// One that is skipped says why on stderr, puts nothing on stdout, and is no +/// failure. +fn pickup(args: PickupArgs) -> ExitCode { + let config = match user_config() { + Ok(config) => config, + Err(failure) => return failure, + }; + // First, so no interrupt can end the Pickup run once it has taken an + // issue. + if let Err(error) = interrupt::install() { + return failure(&error); + } + let taken = match pickup::run(args.base.as_deref()) { + Ok(pickup::Outcome::Taken(taken)) => taken, + Ok(pickup::Outcome::Skipped(skipped)) => return outcome(Ok(skipped)), + Err(error) => return failure(&error), + }; + let email = args.email.clone().unwrap_or(config.email.default_ask()); + let notification = match asked(email, |to| { + RunNotification::new(to, &config.email, &taken.issue) + }) { + Ok(notification) => notification, + Err(error) => return failure(&error), + }; + // Ignored for an issue that is not a Spec: the command can't know which + // it will take. Left out of the command a Base fix is offered with too, + // as the issue is retried as a Run of its own: another Pickup run never + // takes an issue that was started. + let parallel = args.dispatch.parallel.filter(|_| taken.is_spec); + let ended = dispatched( + &taken.issue, + &taken.base, + &args.dispatch, + args.email.as_ref(), + parallel, + &config, + ); + let code = run_outcome(&ended); + if let Some(notification) = notification { + notification.send(&ended); + } + code +} + +/// The end of the Spec run or Run that an Architect run or a Pickup run +/// dispatches `issue` as, on its Base branch `base`: what `thirdshift <Issue +/// URL>` would start, with the flags `dispatch` and the User config `config` +/// for the rest. It runs as many Tickets at once as `parallel` asks, which is +/// `dispatch`'s unless that is to be ignored. The command it offers a Base +/// fix with is that one on `issue`, with those flags and with `email` as the +/// command gave it. +fn dispatched( + issue: &IssueUrl, + base: &str, + dispatch: &DispatchArgs, + email: Option<&NotificationAsk>, + parallel: Option<NonZeroUsize>, + config: &UserConfig, +) -> Ended { + run::run_to_end( + issue, + dispatch.goal.unwrap_or(config.default_goal()), + &config.logs_dir, + config.launch_pull, + Parallel::new(parallel, config.spec_parallel), + StartedBy::Dispatch { base }, + dispatch.base_fix.clone().unwrap_or_else(|| { + let retry = args::retry_with_base_fix(issue, dispatch.goal, email, parallel); + config.default_base_fix(retry) + }), + ) +} + /// The Run notification `email` asks for, if it asks for one: what `checked` /// makes of the address it gave, or its error if a check fails. fn asked<N>( diff --git a/src/notification.rs b/src/notification.rs index 42ab6a0de..268cc223f 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -7,7 +7,7 @@ use std::time::{Duration, Instant}; use anyhow::Result; -use crate::architect::{self, Outcome}; +use crate::architect::Outcome; use crate::base_fix::Advice; use crate::config::EmailSettings; use crate::email::Resend; @@ -15,6 +15,7 @@ use crate::failed_run::FailedRun; use crate::github; use crate::host; use crate::issue::{IssueUrl, Repo}; +use crate::launch; use crate::progress; use crate::run::Ended; @@ -87,7 +88,7 @@ impl ArchitectNotification { resend, // Left out of the subject if origin names none; the Architect // run's own preflight reports why. - repo: architect::repo().ok(), + repo: launch::repo().ok(), started, }) } @@ -522,7 +523,7 @@ mod tests { fn a_skipped_architect_runs_body_starts_with_why_it_was_skipped() { let body = Body { architect: Some(ArchitectLines::Skipped( - "an Architect run is already running on acme/widgets".to_string(), + "an Architect run or a Pickup run is already running on acme/widgets".to_string(), )), pr_url: None, cause: None, @@ -535,7 +536,7 @@ mod tests { }; assert_eq!( body.text(), - "Skipped: an Architect run is already running on acme/widgets\n\ + "Skipped: an Architect run or a Pickup run is already running on acme/widgets\n\ Host: droplet-1\n\ Took: 0s\n" ); diff --git a/src/pickup.rs b/src/pickup.rs new file mode 100644 index 000000000..eefc9d23a --- /dev/null +++ b/src/pickup.rs @@ -0,0 +1,110 @@ +//! A Pickup run up to the issue it takes: from the Launch directory with no +//! Issue URL, the search for the lowest-numbered Ready issue in the +//! repository, for the command to dispatch. Only one Pickup run or Architect +//! run per repository runs at a time on a machine: one started while another +//! is still running is skipped, before any search. So is one that finds no +//! Ready issue. + +use std::fmt; + +use anyhow::Result; + +use crate::branch; +use crate::claim; +use crate::git::Git; +use crate::github::{self, ListedIssue}; +use crate::issue::{IssueUrl, Repo}; +use crate::launch::{self, AlreadyRunning, Launch, Start}; +use crate::progress; +use crate::spec_run::{self, READY_FOR_AGENT}; + +/// How a Pickup run ended, short of a failure and before any dispatch. +pub enum Outcome { + /// It took this Ready issue, to dispatch. + Taken(Taken), + /// Skipped, having done nothing. + Skipped(Skipped), +} + +/// The Ready issue a Pickup run took. +pub struct Taken { + pub issue: IssueUrl, + /// The Pickup run's Base branch, which the run the issue is dispatched + /// as takes. + pub base: String, + /// Whether it is a Spec, an issue with sub-issues, which is dispatched + /// as a Spec run. + pub is_spec: bool, +} + +/// Why a Pickup run was skipped. Its `Display` is the reason, as the skipped +/// run's one line gives it. +pub enum Skipped { + /// An Architect run or another Pickup run on this repository is still + /// running on this machine, the Spec run or Run it dispatched included. + AlreadyRunning(AlreadyRunning), + /// The repository has no Ready issue. + NoReadyIssue(Repo), +} + +impl fmt::Display for Skipped { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::AlreadyRunning(running) => running.fmt(f), + Self::NoReadyIssue(repo) => write!(f, "no Ready issue on {}", repo.slug()), + } + } +} + +/// Take the lowest-numbered Ready issue in the Launch directory's +/// repository, saying which on stderr. The Base branch is `base`, the branch +/// the command named, whatever the Launch directory has checked out, or +/// without one the branch checked out there. Nothing is changed, on GitHub +/// or in the Launch directory: the run the issue is dispatched as makes the +/// Claim. +/// +/// Once the preflight checks pass, and before any search, the Pickup run is +/// skipped if an Architect run or another Pickup run on the same repository +/// is still running on this machine. Otherwise this process is that +/// repository's one such run until it exits, through whatever it dispatches. +pub fn run(base: Option<&str>) -> Result<Outcome> { + let Launch { + git, repo, base, .. + } = match launch::start(base)? { + Start::Clear(launch) => launch, + Start::AlreadyRunning(running) => { + return Ok(Outcome::Skipped(Skipped::AlreadyRunning(running))); + } + }; + let mut candidates = github::open_issues_labelled(&repo.slug(), READY_FOR_AGENT)?; + candidates.sort_by_key(|candidate| candidate.issue.number); + let mut ready = None; + for candidate in candidates { + if is_ready(&git, &candidate)? { + ready = Some(candidate); + break; + } + } + let Some(ready) = ready else { + return Ok(Outcome::Skipped(Skipped::NoReadyIssue(repo))); + }; + progress::step(format_args!( + "taking Ready issue #{} \"{}\", as thirdshift {} would", + ready.issue.number, ready.title, ready.issue.url + )); + let is_spec = !github::tickets(&ready.issue)?.is_empty(); + Ok(Outcome::Taken(Taken { + issue: ready.issue, + base, + is_spec, + })) +} + +/// Whether `candidate`, an open issue labelled `ready-for-agent` in the +/// repository of the Launch directory `launch`, is a Ready issue: it has no +/// label that makes an Unready Ticket, no Claim, and was never started. +fn is_ready(launch: &Git, candidate: &ListedIssue) -> Result<bool> { + Ok(spec_run::unready_label(&candidate.labels).is_none() + && !claim::is_on(&candidate.labels) + && !branch::started(launch, &candidate.issue)?) +} diff --git a/src/preflight.rs b/src/preflight.rs index 5cd1cbe92..ac3b40c6c 100644 --- a/src/preflight.rs +++ b/src/preflight.rs @@ -1,5 +1,5 @@ -//! Pre-flight checks: everything that must hold before a Run or an Architect -//! run creates anything. +//! Pre-flight checks: everything that must hold before a Run, an Architect +//! run or a Pickup run creates anything. use anyhow::{Result, bail}; diff --git a/src/run.rs b/src/run.rs index 06f1a7057..3eed3fe50 100644 --- a/src/run.rs +++ b/src/run.rs @@ -71,9 +71,10 @@ pub enum StartedBy<'a> { /// `thirdshift <Issue URL>`: the Base branch is the branch checked out /// in the Launch directory. Command, - /// An Architect run, dispatching its plan: the Base branch is the - /// Architect run's, whatever the Launch directory has checked out. - ArchitectRun { base: &'a str }, + /// An Architect run, dispatching its plan, or a Pickup run, dispatching + /// the Ready issue it took: the Base branch is that run's, whatever the + /// Launch directory has checked out. + Dispatch { base: &'a str }, /// Another thirdshift, as this child Run, a Ticket's Run in a Spec run or /// a Base fix: the Base branch is the one the child Run was given. Child(&'a Kind), @@ -84,7 +85,7 @@ impl<'a> StartedBy<'a> { fn child(self) -> Option<&'a Kind> { match self { StartedBy::Child(kind) => Some(kind), - StartedBy::Command | StartedBy::ArchitectRun { .. } => None, + StartedBy::Command | StartedBy::Dispatch { .. } => None, } } @@ -99,7 +100,7 @@ impl<'a> StartedBy<'a> { fn given_base(self) -> Option<&'a str> { match self { StartedBy::Command => None, - StartedBy::ArchitectRun { base } => Some(base), + StartedBy::Dispatch { base } => Some(base), StartedBy::Child(kind) => Some(kind.base()), } } @@ -145,10 +146,10 @@ pub fn run_to_end( /// The Base branch `started_by` gave the Run, if it gave one, stands in for /// the checked-out branch as the Base branch: the Spec branch or the Base /// branch of the Run that started a child Run, or the Base branch of the -/// Architect run that dispatched this one. Unless the Run is a child Run, an -/// issue with sub-issues is a Spec, taken on by a Spec run instead, whose -/// Spec branch is picked like an Issue branch, running as many Tickets at -/// once as `parallel` says. A `parallel` the command asked for on an issue +/// Architect run or the Pickup run that dispatched this one. Unless the Run +/// is a child Run, an issue with sub-issues is a Spec, taken on by a Spec run +/// instead, whose Spec branch is picked like an Issue branch, running as many +/// Tickets at once as `parallel` says. A `parallel` the command asked for on an issue /// with no sub-issues fails before any work, as does a Spec whose Tickets are /// all closed with no Spec branch to continue. /// diff --git a/src/spec_run.rs b/src/spec_run.rs index 8e12d7ffe..485c6af66 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -345,11 +345,12 @@ fn next_ready( .min() } -/// The first of the Unready Ticket labels among an issue's `labels`, if any. +/// The first of the Unready Ticket labels among an issue's `labels`, if any, +/// whatever its case there: GitHub's label names are case-insensitive. pub fn unready_label(labels: &[String]) -> Option<&'static str> { UNREADY_LABELS .into_iter() - .find(|label| labels.iter().any(|name| name == label)) + .find(|label| labels.iter().any(|name| name.eq_ignore_ascii_case(label))) } /// A line on each Ticket that landed in this Spec run, with its PR, and on diff --git a/tests/architect_run.rs b/tests/architect_run.rs index c3e2163b1..461eee6ea 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -13,8 +13,9 @@ //! The Base branch is the branch checked out in the Launch directory, or the //! one `base <branch>` names, whatever is checked out there, which the run //! the plan is dispatched as takes as its Base branch too. -//! An Architect run started while another on the repository is still running, -//! or while an Architect plan is still open there, is skipped. +//! An Architect run started while another on the repository, or a Pickup run, +//! is still running, or while an Architect plan is still open there, is +//! skipped. mod support; @@ -607,7 +608,7 @@ fn a_detached_head_is_rejected_before_any_work() { scenario.assert_rejected_before_any_work( &result, - "HEAD is detached; check out the branch the Architecture review should scan, \ + "HEAD is detached; check out the branch the work should be based on, \ or name it with base <branch>", ); } @@ -1682,7 +1683,8 @@ fn a_dispatched_spec_run_that_fails_sends_one_notification_with_each_tickets_out /// What a skipped Architect run says when another is running on the /// scenario's repository. -const ALREADY_RUNNING: &str = "thirdshift: an Architect run is already running on acme/widgets\n"; +const ALREADY_RUNNING: &str = + "thirdshift: an Architect run or a Pickup run is already running on acme/widgets\n"; /// Assert the Architect run was skipped as one is already running: exit 0, /// that line alone on stderr, and nothing on stdout. @@ -1871,7 +1873,7 @@ fn a_skipped_architect_run_sends_one_notification_with_skipped_in_its_subject_an assert_eq!(subject, "[thirdshift] acme/widgets Architect run: skipped"); assert!( text.starts_with( - "Skipped: an Architect run is already running on acme/widgets\n\ + "Skipped: an Architect run or a Pickup run is already running on acme/widgets\n\ Host: " ), "{text}" diff --git a/tests/command_surface.rs b/tests/command_surface.rs index b2bf3811c..6138a2b38 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -15,6 +15,8 @@ fn assert_help_text(text: &str) { "thirdshift architect [<focus>]", "thirdshift architect [<focus>] --plan-only", "thirdshift architect base <branch> [<focus>]", + "thirdshift pickup", + "thirdshift pickup base <branch>", "thirdshift email-test [<address>]", "thirdshift setup", "thirdshift update", @@ -92,8 +94,8 @@ fn help_documents_architect_its_focus_the_dispatch_plan_only_and_when_an_archite "thirdshift architect \"the Spec run\"", "A review that finds no Strong candidate publishes no plan", "thirdshift prints that issue's URL instead, changing no label", - "Only one Architect run per repository runs at a time on a machine", - "is skipped: it prints an Architect run is already running on <owner>/<repo>, does nothing else and exits 0", + "Only one Architect run or Pickup run per repository runs at a time on a machine", + "is skipped: it prints an Architect run or a Pickup run is already running on <owner>/<repo>, does nothing else and exits 0", "A skipped run still sends its Run notification, with the outcome skipped", "swaps its needs-triage label for ready-for-agent, labels it architect-plan", "creating the label if the repository lacks it", @@ -195,7 +197,7 @@ fn help_documents_base_for_an_architect_run() { "<branch> must exist on origin, with no local copy of it ahead", "launch.pull updates the clone only when <branch> is the branch checked out", "base goes with --plan-only too", - "base is for architect only", + "base is for architect and pickup only", "Without base, the Base branch is the branch checked out", "thirdshift architect base main", ] { @@ -203,6 +205,101 @@ fn help_documents_base_for_an_architect_run() { } } +#[test] +fn help_documents_pickup_the_ready_issue_the_dispatch_and_when_a_pickup_run_is_skipped() { + let scenario = Scenario::new(); + + let help = unwrapped_help(&scenario); + + for mention in [ + "pickup starts a Pickup run from the clone, with no Issue URL", + "takes the lowest-numbered Ready issue in the repository and dispatches it as thirdshift <Issue URL> would: a Spec run on a Spec, a Run otherwise", + "A Ready issue is an open issue labelled ready-for-agent", + "none of ready-for-human, needs-info, wontfix and needs-triage", + "is not in-progress", + "was never started: no Issue branch for it is on origin, and no pull request from one exists, open, merged or closed", + "The Pickup run ends as that run does, with its exit code and its PR's URL", + "merge, --no-merge, base-fix, --no-base-fix, --email, --no-email and parallel <n> apply to that run, as do the User config's defaults", + "parallel <n> is ignored when the issue is not a Spec", + "base <branch> names the Pickup run's Base branch as it does an Architect run's", + "pickup takes nothing else: no focus and no --plan-only", + "A Pickup run is skipped, exiting 0 with nothing on stdout and one line on stderr saying why", + "when the repository has no Ready issue", + "while an Architect run or another Pickup run on the same repository is still running on this machine", + ] { + assert!(help.contains(mention), "help lacks {mention:?}: {help}"); + } +} + +#[test] +fn pickup_with_arguments_it_cant_use_prints_an_error_and_the_help_to_stderr() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + + for (args, error) in [ + ( + vec!["pickup", "the Spec run"], + "unexpected argument after pickup: the Spec run", + ), + ( + vec!["pickup", "--plan-only"], + "unexpected argument after pickup: --plan-only", + ), + ( + vec![ + "pickup", + "merge", + "https://github.com/acme/widgets/issues/7", + ], + "unexpected argument after pickup: https://github.com/acme/widgets/issues/7", + ), + ( + vec!["pickup", "merge", "--merge"], + "repeated argument: --merge", + ), + ( + vec!["pickup", "merge", "no-merge"], + "merge and no-merge can't be used together", + ), + ( + vec!["pickup", "base-fix", "--no-base-fix"], + "base-fix and no-base-fix can't be used together", + ), + ( + vec!["pickup", "email", "--no-email"], + "email and no-email can't be used together", + ), + ( + vec!["pickup", "parallel", "0"], + "parallel must be followed by a whole number from 1 up, not 0", + ), + ( + vec!["pickup", "parallel", "2", "--parallel", "2"], + "repeated argument: --parallel", + ), + (vec!["pickup", "base"], "base must be followed by a branch"), + ( + vec!["pickup", "base", "main", "--base", "main"], + "repeated argument: --base", + ), + ] { + let result = scenario.run(&args); + + assert_argument_error(&scenario, &result, error); + assert!(scenario.claude_calls().is_empty(), "{args:?} started a Run"); + assert_eq!(scenario.issue_labels(7), ["ready-for-agent"], "{args:?}"); + } +} + +#[test] +fn pickup_is_a_command_only_as_the_first_argument() { + let scenario = Scenario::new(); + + let result = scenario.run(&["merge", "pickup"]); + + assert_argument_error(&scenario, &result, "not a GitHub issue URL: pickup"); +} + #[test] fn help_documents_the_claim() { let scenario = Scenario::new(); diff --git a/tests/pickup_run.rs b/tests/pickup_run.rs new file mode 100644 index 000000000..e4090e6a1 --- /dev/null +++ b/tests/pickup_run.rs @@ -0,0 +1,768 @@ +//! Pickup runs: `thirdshift pickup` takes the lowest-numbered Ready issue in +//! the repository, an open issue labelled `ready-for-agent` with no label +//! that makes an Unready Ticket, no Claim and nothing started on it, and +//! dispatches it as `thirdshift <Issue URL>` would, a Spec run or a Run, +//! ending as that does. With no Ready issue, or while an Architect run or +//! another Pickup run on the repository is still running, it is skipped. + +mod support; + +use std::fs; + +use support::resend::ResendStandIn; +use support::{REPO, RunResult, Scenario}; + +/// The label of an issue a Pickup run may take. +const READY_FOR_AGENT: &str = "ready-for-agent"; + +/// The label of a Claimed issue, which the dispatched run swaps the issue's +/// `ready-for-agent` for. +const IN_PROGRESS: &str = "in-progress"; + +/// Make issue `number` open and labelled `ready-for-agent` on the fake +/// GitHub, with `more` labels after it. +fn ready_issue(scenario: &Scenario, number: u32, more: &[&str]) { + scenario.issue_is(number, "OPEN"); + scenario.issue_labelled(number, &[&[READY_FOR_AGENT], more].concat()); +} + +/// A script in which the agent for issue `issue` commits its work and opens +/// its PR into `base`, leaving the pushing to thirdshift. +fn agent_opens_pr(issue: u32, base: &str) -> String { + format!( + r#" +echo "{issue}" > issue-{issue}.txt +git add issue-{issue}.txt +git commit -q -m "Work on {issue}" +gh pr create --base {base} --head issue-{issue} --title "Work on {issue}" --body "Closes #{issue}" +"# + ) +} + +/// The newest pull request from `head` on the fake GitHub. +fn pr_from(scenario: &Scenario, head: &str) -> serde_json::Value { + let prs = scenario.gh_state()["prs"].clone(); + let from_head = prs + .as_array() + .unwrap() + .iter() + .rfind(|pr| pr["head"] == head); + from_head + .unwrap_or_else(|| panic!("no PR from {head}: {prs}")) + .clone() +} + +/// Assert the Pickup run ended with `pr` in `outcome`, as a Run or a Spec +/// run that reached its goal does: exit 0, the PR's URL alone on stdout, and +/// the outcome as the last line on stderr. +fn assert_ended_with_pr(result: &RunResult, pr: &serde_json::Value, outcome: &str) { + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let url = pr["url"].as_str().unwrap(); + assert_eq!(result.stdout, format!("{url}\n")); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: PR {url} is {outcome}").as_str()), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn the_lower_numbered_of_two_ready_issues_is_run_and_the_other_left_untouched() { + let scenario = Scenario::new(); + ready_issue(&scenario, 9, &["bug"]); + ready_issue(&scenario, 7, &["bug"]); + scenario.agent_does_for(7, &agent_opens_pr(7, "main")); + + let result = scenario.run(&["pickup"]); + + let pr = pr_from(&scenario, "issue-7"); + assert_ended_with_pr(&result, &pr, "ready for review"); + assert_eq!(pr["base"], "main"); + assert_eq!(pr["state"], "OPEN"); + assert_eq!(scenario.issue_labels(7), ["bug", IN_PROGRESS]); + assert_eq!(scenario.issue_labels(9), [READY_FOR_AGENT, "bug"]); + assert!(scenario.origin_log("issue-9").is_none()); + let calls = scenario.claude_calls(); + assert_eq!(calls.len(), 1, "sessions: {calls:?}"); + let prompt = calls[0]["prompt"].as_str().unwrap(); + assert!( + prompt.starts_with(&format!( + "/thirdshift:implement {}\n", + scenario.issue_url(7) + )), + "{prompt}" + ); + scenario.assert_cleaned_up("issue-7"); +} + +/// What a skipped Pickup run says when the scenario's repository has no +/// Ready issue. +const NO_READY_ISSUE: &str = "thirdshift: no Ready issue on acme/widgets\n"; + +/// Assert the Pickup run was skipped with `reason` as its one line: exit 0, +/// that line alone on stderr, nothing on stdout, and no session started. +fn assert_skipped(scenario: &Scenario, result: &RunResult, reason: &str) { + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stderr, reason); + assert_eq!(result.stdout, ""); + assert!(scenario.claude_calls().is_empty(), "a session was started"); +} + +#[test] +fn a_progress_line_names_the_issue_taken_before_it_is_dispatched() { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.issue_titled(7, "Sharpen the widgets"); + scenario.agent_does_for(7, &agent_opens_pr(7, "main")); + + let result = scenario.run(&["pickup"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let url = scenario.issue_url(7); + let taking = format!( + "thirdshift: taking Ready issue #7 \"Sharpen the widgets\", as thirdshift {url} would\n" + ); + assert!( + result.stderr.starts_with(&taking), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn an_issue_without_ready_for_agent_is_never_taken_and_with_no_ready_issue_the_pass_is_skipped() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["bug"]); + scenario.issue_is(8, "OPEN"); + let github = scenario.gh_state(); + + let result = scenario.run(&["pickup"]); + + assert_skipped(&scenario, &result, NO_READY_ISSUE); + assert_eq!(scenario.gh_state(), github); +} + +#[test] +fn a_closed_issue_labelled_ready_for_agent_is_not_taken() { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.issue_is(7, "CLOSED"); + + let result = scenario.run(&["pickup"]); + + assert_skipped(&scenario, &result, NO_READY_ISSUE); +} + +#[test] +fn an_issue_that_also_has_a_label_that_makes_an_unready_ticket_is_not_taken() { + // Whatever its case: GitHub's label names are case-insensitive. + for unready in [ + "ready-for-human", + "needs-info", + "wontfix", + "needs-triage", + "Needs-Info", + ] { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[unready]); + + let result = scenario.run(&["pickup"]); + + assert_skipped(&scenario, &result, NO_READY_ISSUE); + assert_eq!(scenario.issue_labels(7), [READY_FOR_AGENT, unready]); + } +} + +#[test] +fn an_issue_labelled_in_progress_is_not_taken() { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[IN_PROGRESS]); + + let result = scenario.run(&["pickup"]); + + assert_skipped(&scenario, &result, NO_READY_ISSUE); + assert_eq!(scenario.issue_labels(7), [READY_FOR_AGENT, IN_PROGRESS]); +} + +#[test] +fn an_issue_with_an_issue_branch_on_origin_is_not_taken() { + for branch in ["issue-7", "issue-7-branch-2"] { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.origin_has_branch(branch, "main", &["Earlier work"]); + + let result = scenario.run(&["pickup"]); + + assert_skipped(&scenario, &result, NO_READY_ISSUE); + assert_eq!(scenario.issue_labels(7), [READY_FOR_AGENT], "{branch}"); + } +} + +#[test] +fn an_issue_with_a_pull_request_from_an_issue_branch_in_any_state_is_not_taken() { + for state in ["OPEN", "MERGED", "CLOSED"] { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.github_has_pr("issue-7", "main", state); + + let result = scenario.run(&["pickup"]); + + assert_skipped(&scenario, &result, NO_READY_ISSUE); + assert_eq!(scenario.issue_labels(7), [READY_FOR_AGENT], "{state}"); + } +} + +#[test] +fn an_issue_that_was_started_is_passed_over_for_the_next_ready_issue() { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.github_has_pr("issue-7", "main", "CLOSED"); + ready_issue(&scenario, 8, &["wontfix"]); + ready_issue(&scenario, 9, &[IN_PROGRESS]); + // Neither #7's Issue branch nor its pull request is one of #70's. + ready_issue(&scenario, 70, &[]); + scenario.agent_does_for(70, &agent_opens_pr(70, "main")); + + let result = scenario.run(&["pickup"]); + + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-70"), "ready for review"); + assert_eq!(scenario.issue_labels(7), [READY_FOR_AGENT]); + assert_eq!(scenario.issue_labels(70), [IN_PROGRESS]); +} + +/// A Ready issue, #7, that is a Spec with two Tickets that don't block each +/// other, #8 and #9. Each Ticket's session opens its PR into the Spec branch, +/// #9's after doing `before_ticket_9`. +fn ready_spec(before_ticket_9: &str) -> Scenario { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.spec_has_tickets(7, &[(8, &[]), (9, &[])]); + scenario.agent_does_for(8, &agent_opens_pr(8, "issue-7")); + scenario.agent_does_for( + 9, + &format!("{before_ticket_9}\n{}", agent_opens_pr(9, "issue-7")), + ); + scenario +} + +/// A Ready issue, #7, with no sub-issues, whose session opens its PR into +/// `main`. +fn ready_ticket() -> Scenario { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.agent_does_for(7, &agent_opens_pr(7, "main")); + scenario +} + +/// The issue number each agent session was for, the first Issue URL its +/// prompt names, in order. +fn sessions(scenario: &Scenario) -> Vec<String> { + scenario + .claude_calls() + .iter() + .map(|call| { + let prompt = call["prompt"].as_str().unwrap(); + let (_, after) = prompt.split_once("/issues/").unwrap(); + after.chars().take_while(char::is_ascii_digit).collect() + }) + .collect() +} + +#[test] +fn a_ready_issue_with_sub_issues_is_run_as_a_spec_run_and_ends_as_that_spec_run_does() { + let scenario = ready_spec(""); + + let result = scenario.run(&["pickup"]); + + let spec_pr = pr_from(&scenario, "issue-7"); + assert_ended_with_pr(&result, &spec_pr, "ready for review"); + assert_eq!(spec_pr["base"], "main"); + assert_eq!(spec_pr["state"], "OPEN"); + assert_eq!(spec_pr["isDraft"], false); + assert_eq!(scenario.issue_labels(7), [IN_PROGRESS]); + let mut sessions = sessions(&scenario); + assert_eq!(sessions.pop().as_deref(), Some("7"), "the Spec review"); + sessions.sort(); + assert_eq!(sessions, ["8", "9"]); + for ticket in [8, 9] { + let pr = pr_from(&scenario, &format!("issue-{ticket}")); + assert_eq!(pr["base"], "issue-7"); + assert_eq!(pr["state"], "MERGED"); + } + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn a_dispatched_run_that_fails_fails_the_pickup_run_as_a_failed_run_does() { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.agent_does_for(7, "echo work > work.txt\nexit 1"); + + let result = scenario.run(&["pickup"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, ""); + let last = result.stderr.lines().last().unwrap(); + assert!( + last.starts_with("thirdshift: session log: ") && last.ends_with("-implement.jsonl"), + "stderr: {}", + result.stderr + ); + assert!(scenario.origin_log("issue-7").is_some(), "work not pushed"); +} + +#[test] +fn parallel_reaches_a_dispatched_spec_run() { + // #9's session only succeeds if #8 landed on the Spec branch before it + // started, as it has only when the Tickets run one at a time. + let scenario = ready_spec("test -f issue-8.txt"); + + let result = scenario.run(&["pickup", "parallel", "1"]); + + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); + assert_eq!(sessions(&scenario), ["8", "9", "7"]); +} + +#[test] +fn parallel_is_ignored_without_error_for_an_issue_that_is_not_a_spec() { + for flag in ["parallel", "--parallel"] { + let scenario = ready_ticket(); + + let result = scenario.run(&["pickup", flag, "2"]); + + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); + } +} + +#[test] +fn merge_and_no_merge_and_the_user_configs_default_reach_the_dispatched_run() { + for ready in [ready_ticket, || ready_spec("")] { + for (config, args, outcome, state) in [ + ("", vec!["pickup"], "ready for review", "OPEN"), + ("", vec!["pickup", "merge"], "merged", "MERGED"), + ("", vec!["pickup", "--merge"], "merged", "MERGED"), + ( + "[merge]\nalways = true\n", + vec!["pickup"], + "merged", + "MERGED", + ), + ( + "[merge]\nalways = true\n", + vec!["pickup", "no-merge"], + "ready for review", + "OPEN", + ), + ( + "[merge]\nalways = true\n", + vec!["pickup", "--no-merge"], + "ready for review", + "OPEN", + ), + ] { + let scenario = ready(); + scenario.user_config_is(config); + + let result = scenario.run(&args); + + let pr = pr_from(&scenario, "issue-7"); + assert_ended_with_pr(&result, &pr, outcome); + assert_eq!(pr["state"], state, "{args:?} with {config:?}"); + } + } +} + +const RED: &str = r#"[{"name": "test", "conclusion": "failure"}]"#; + +const GREEN: &str = r#"[{"name": "test", "conclusion": "success"}]"#; + +/// A Ready issue, #7, whose Run opens its PR with `test` red on its head and +/// on `main`: an Inherited failure. The agent for the Base fix issue, #8, the +/// next issue, fixes it. +fn ready_ticket_that_inherits_a_failure() -> Scenario { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.agent_does_for( + 7, + &format!( + r#"{} +gh fake checks "$(git rev-parse HEAD)" '{RED}' +gh fake checks "$(git rev-parse origin/main)" '{RED}' +"#, + agent_opens_pr(7, "main") + ), + ); + scenario.agent_does_for( + 8, + &format!( + r#" +echo "fixed" > ci-fix.txt +git add ci-fix.txt +git commit -q -m "Fix CI on main" +gh pr create --base main --head issue-8 --title "Fix CI on main" --body "Closes #8" +gh fake checks "$(git rev-parse HEAD)" '{GREEN}' +"# + ), + ); + scenario +} + +#[test] +fn base_fix_and_no_base_fix_and_the_user_configs_default_reach_the_dispatched_run() { + for (config, args, fixed) in [ + ("", vec!["pickup"], false), + ("", vec!["pickup", "base-fix"], true), + ("", vec!["pickup", "--base-fix"], true), + ("[base]\nfix = true\n", vec!["pickup"], true), + ("[base]\nfix = true\n", vec!["pickup", "no-base-fix"], false), + ( + "[base]\nfix = true\n", + vec!["pickup", "--no-base-fix"], + false, + ), + ] { + let scenario = ready_ticket_that_inherits_a_failure(); + scenario.user_config_is(config); + let red_base = scenario.origin_git(&["rev-parse", "main"]); + + let result = scenario.run(&args); + + let started = result.stderr.contains( + "thirdshift: starting Base fix #8 into main: https://github.com/acme/widgets/issues/8\n", + ); + assert_eq!( + started, fixed, + "{args:?} with {config:?}: {}", + result.stderr + ); + if fixed { + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); + assert_eq!(pr_from(&scenario, "issue-8")["state"], "MERGED"); + } else { + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let cause = format!( + "thirdshift: CI red on test, which also fails on main at {}; fix main first\n", + &red_base[..7] + ); + assert!(result.stderr.contains(&cause), "stderr: {}", result.stderr); + } + } +} + +#[test] +fn a_run_that_fails_on_an_inherited_failure_offers_the_command_that_retries_its_issue_by_hand() { + let scenario = ready_ticket_that_inherits_a_failure(); + + let result = scenario.run(&["pickup", "merge", "parallel", "2", "base", "main"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + // Not a Spec, so without the `parallel` that would fail its Run by hand. + let retry = format!("thirdshift {} merge base-fix\n", scenario.issue_url(7)); + assert!(result.stderr.contains(&retry), "stderr: {}", result.stderr); +} + +#[test] +fn base_makes_the_named_branch_the_dispatched_runs_base_branch_whatever_is_checked_out() { + for (flag, detached) in [("base", false), ("--base", true)] { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.agent_does_for(7, &agent_opens_pr(7, "develop")); + scenario.origin_has_branch("develop", "main", &["Develop work"]); + if detached { + scenario.launch_git(&["checkout", "-q", "--detach"]); + } + let checked_out = scenario.launch_git(&["branch", "--show-current"]); + + let result = scenario.run(&["pickup", flag, "develop"]); + + let pr = pr_from(&scenario, "issue-7"); + assert_ended_with_pr(&result, &pr, "ready for review"); + assert_eq!(pr["base"], "develop", "detached: {detached}"); + assert_eq!( + scenario.origin_log("issue-7").unwrap(), + ["Work on 7", "Develop work", "Initial commit"], + "detached: {detached}" + ); + assert_eq!( + scenario.launch_git(&["branch", "--show-current"]), + checked_out + ); + scenario.assert_cleaned_up("issue-7"); + } +} + +#[test] +fn base_makes_the_named_branch_the_dispatched_spec_runs_base_branch() { + let scenario = ready_spec(""); + scenario.origin_has_branch("develop", "main", &["Develop work"]); + + let result = scenario.run(&["pickup", "base", "develop", "merge"]); + + let spec_pr = pr_from(&scenario, "issue-7"); + assert_ended_with_pr(&result, &spec_pr, "merged"); + assert_eq!(spec_pr["base"], "develop"); + assert_eq!( + scenario.origin_file("develop", "issue-8.txt").as_deref(), + Some("8\n") + ); + assert!(scenario.origin_file("main", "issue-8.txt").is_none()); +} + +/// Assert the Pickup run was stopped by a preflight check with `message`, +/// exit 1, before any work: no session, and no call to GitHub, so no label +/// was read or changed. +fn assert_stopped_by_preflight(scenario: &Scenario, result: &RunResult, message: &str) { + scenario.assert_rejected_before_any_work(result, message); + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(scenario.gh_calls(), Vec::<Vec<String>>::new()); + assert_eq!(scenario.issue_labels(7), [READY_FOR_AGENT]); +} + +#[test] +fn a_detached_head_without_base_stops_the_pass_before_any_label_changes() { + let scenario = ready_ticket(); + scenario.launch_git(&["checkout", "-q", "--detach"]); + + let result = scenario.run(&["pickup"]); + + assert_stopped_by_preflight( + &scenario, + &result, + "thirdshift: HEAD is detached; check out the branch the work should be based on, \ + or name it with base <branch>\n", + ); +} + +#[test] +fn a_base_branch_ahead_of_origin_stops_the_pass_before_any_label_changes() { + let scenario = ready_ticket(); + scenario.commit_locally("local.txt", "local\n", "Local work"); + + let result = scenario.run(&["pickup"]); + + assert_stopped_by_preflight( + &scenario, + &result, + "local main is 1 commit(s) ahead of origin/main; push them first", + ); +} + +#[test] +fn a_named_branch_that_is_not_on_origin_stops_the_pass_before_any_label_changes() { + let scenario = ready_ticket(); + + let result = scenario.run(&["pickup", "base", "nowhere"]); + + assert_stopped_by_preflight( + &scenario, + &result, + "base branch nowhere does not exist on origin; push it first", + ); +} + +#[test] +fn a_missing_git_identity_stops_the_pass_before_any_label_changes() { + let scenario = ready_ticket(); + scenario.git_email_is(None); + + let result = scenario.run(&["pickup"]); + + assert_stopped_by_preflight(&scenario, &result, "git user.email is not set"); +} + +#[test] +fn an_origin_that_is_not_on_github_stops_the_pass_before_any_label_changes() { + let scenario = ready_ticket(); + scenario.set_origin_url("https://gitlab.com/acme/widgets.git"); + + let result = scenario.run(&["pickup"]); + + assert_stopped_by_preflight( + &scenario, + &result, + "origin https://gitlab.com/acme/widgets.git is not a GitHub repository", + ); +} + +/// What a skipped Pickup run or Architect run says when another of either is +/// running on the scenario's repository. +const ALREADY_RUNNING: &str = + "thirdshift: an Architect run or a Pickup run is already running on acme/widgets\n"; + +/// A script in which the agent touches `started` in the scenario root, then +/// waits there until the test touches `release`, or until the scenario is +/// gone, as after a test that failed while holding it. +const AGENT_WAITS_FOR_RELEASE: &str = r#" +root=$(dirname "$FAKE_CLAUDE_RECORD") +touch "$root/started" +while [ -d "$root" ] && [ ! -e "$root/release" ]; do sleep 0.05; done +"#; + +/// Let an agent held by [`AGENT_WAITS_FOR_RELEASE`] go on. +fn release(scenario: &Scenario) { + fs::write(scenario.path("release"), "").unwrap(); +} + +#[test] +fn a_pickup_run_is_skipped_while_an_architect_run_on_the_repository_is_still_running() { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + scenario.agent_does(AGENT_WAITS_FOR_RELEASE); + let architect = scenario.run_until(&["architect", "--plan-only"], &[], "started"); + let (github, gh_calls) = (scenario.gh_state(), scenario.gh_calls()); + + let result = scenario.run(&["pickup"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stderr, ALREADY_RUNNING); + assert_eq!(result.stdout, ""); + assert_eq!(scenario.claude_calls().len(), 1, "a session was started"); + assert_eq!(scenario.gh_state(), github); + assert_eq!(scenario.gh_calls(), gh_calls); + assert_eq!(scenario.entries("work"), [REPO, "widgets-architect"]); + release(&scenario); + architect.finish(); +} + +#[test] +fn a_pickup_run_or_an_architect_run_is_skipped_while_a_pickup_runs_dispatched_run_is_still_going() { + for second in ["pickup", "architect"] { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + ready_issue(&scenario, 9, &[]); + scenario.agent_does_for( + 7, + &format!("{AGENT_WAITS_FOR_RELEASE}{}", agent_opens_pr(7, "main")), + ); + let first = scenario.run_until(&["pickup"], &[], "started"); + let (github, gh_calls) = (scenario.gh_state(), scenario.gh_calls()); + + let skipped = scenario.run(&[second]); + + assert_eq!(skipped.code, Some(0), "stderr: {}", skipped.stderr); + assert_eq!(skipped.stderr, ALREADY_RUNNING, "{second}"); + assert_eq!(skipped.stdout, ""); + assert_eq!(scenario.claude_calls().len(), 1, "a session was started"); + assert_eq!(scenario.gh_state(), github); + assert_eq!(scenario.gh_calls(), gh_calls); + release(&scenario); + let first = first.finish(); + assert_ended_with_pr(&first, &pr_from(&scenario, "issue-7"), "ready for review"); + assert_eq!(scenario.issue_labels(9), [READY_FOR_AGENT]); + } +} + +#[test] +fn once_a_pickup_run_has_ended_the_next_takes_the_next_ready_issue() { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + ready_issue(&scenario, 9, &[]); + scenario.agent_does_for(7, &agent_opens_pr(7, "main")); + scenario.agent_does_for(9, &agent_opens_pr(9, "main")); + let first = scenario.run(&["pickup"]); + assert_ended_with_pr(&first, &pr_from(&scenario, "issue-7"), "ready for review"); + + let second = scenario.run(&["pickup"]); + + assert_ended_with_pr(&second, &pr_from(&scenario, "issue-9"), "ready for review"); + let third = scenario.run(&["pickup"]); + assert_eq!(third.code, Some(0), "stderr: {}", third.stderr); + assert_eq!(third.stderr, NO_READY_ISSUE); + assert_eq!(third.stdout, ""); + assert_eq!(sessions(&scenario), ["7", "9"]); +} + +const KEY: &str = "re_test_123"; + +const ACCEPTED: &str = r#"{"id":"49a3999c-0ce1-4ea6-ab68-afcd6dc2e794"}"#; + +/// Run thirdshift with `args` against `resend`, with a Resend API key in the +/// environment. +fn run_with_resend(scenario: &Scenario, resend: &ResendStandIn, args: &[&str]) -> RunResult { + let env = [ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", KEY), + ]; + scenario.run_with_env(args, &env) +} + +const EMAIL_ALWAYS: &str = "[email]\nalways = true\nto = \"config@example.com\"\n"; + +#[test] +fn the_dispatched_run_sends_its_run_notification_as_it_would_by_hand() { + for (config, args, to) in [ + ( + "", + vec!["pickup", "email", "me@example.com"], + Some("me@example.com"), + ), + (EMAIL_ALWAYS, vec!["pickup"], Some("config@example.com")), + (EMAIL_ALWAYS, vec!["pickup", "--no-email"], None), + ("", vec!["pickup"], None), + ] { + let scenario = ready_ticket(); + scenario.issue_titled(7, "Sharpen the widgets"); + scenario.user_config_is(config); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &args); + + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); + let requests = resend.requests(); + let sent: Vec<_> = requests.iter().map(|request| &request.body).collect(); + match to { + Some(to) => { + assert_eq!(sent.len(), 1, "{args:?} with {config:?}: {sent:?}"); + assert_eq!(sent[0]["to"], to); + assert_eq!( + sent[0]["subject"], + "[thirdshift] acme/widgets#7 Sharpen the widgets: ready for review" + ); + } + None => assert!(sent.is_empty(), "{args:?} with {config:?}: {sent:?}"), + } + } +} + +#[test] +fn a_skipped_pickup_run_sends_no_run_notification() { + let scenario = Scenario::new(); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["pickup", "email", "me@example.com"]); + + assert_skipped(&scenario, &result, NO_READY_ISSUE); + assert!(resend.requests().is_empty()); +} + +#[test] +fn asked_for_a_notification_with_no_address_known_the_issue_taken_is_left_as_it_was() { + let scenario = ready_ticket(); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["pickup", "--email"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert!( + result.stderr.contains("no email address"), + "stderr: {}", + result.stderr + ); + assert_eq!(result.stdout, ""); + assert_eq!(scenario.issue_labels(7), [READY_FOR_AGENT]); + assert!(scenario.claude_calls().is_empty(), "a session was started"); + assert!(resend.requests().is_empty()); +} + +#[test] +fn the_user_configs_spec_parallel_reaches_a_dispatched_spec_run() { + // As in `parallel_reaches_a_dispatched_spec_run`: one Ticket at a time. + let scenario = ready_spec("test -f issue-8.txt"); + scenario.user_config_is("[spec]\nparallel = 1\n"); + + let result = scenario.run(&["pickup"]); + + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); + assert_eq!(sessions(&scenario), ["8", "9", "7"]); +}