diff --git a/README.md b/README.md index 7b8c9d9f0..bfa26ae03 100644 --- a/README.md +++ b/README.md @@ -139,7 +139,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](#one-at-a-time) Architect run prints nothing there. +`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). 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. @@ -418,28 +418,28 @@ A second focus, a repeated flag, `merge` with `no-merge`, `base-fix` with `no-ba 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. 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 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. 5. Checks the plan: the issue is in this repository, is open, was created after the Architect run started, and carries no other label that says it is not agent work (`ready-for-human`, `needs-info` or `wontfix`). -6. Marks the plan ready, in one request: `needs-triage` is swapped for `ready-for-agent`, and its other labels are kept. A Spec's Tickets are left as the review labelled them. +6. Marks the plan ready, in one request: `needs-triage` is swapped for `ready-for-agent`, the plan is labelled `architect-plan`, which makes it the **Architect plan**, and its other labels are kept. thirdshift adds that label, never the agent, and first creates it if the repository has none. A Spec's Tickets are left as the review labelled them, without `architect-plan`. 7. Dispatches the plan, unless given `--plan-only`, exactly as `thirdshift ` would from the same clone on the Architect run's Base branch: a [Spec run](#spec-runs) when the plan has sub-issues, a Run otherwise. Its Base branch is the Architect run's, so with `base ` it is ``, not the branch checked out. `merge` and `no-merge` apply to the Spec PR or the Run's pull request, `parallel ` to the Spec run, and `base-fix` and `no-base-fix` to whether that run may start a [Base fix](#base-fix), as if given to that command, and the [User config](#user-config) sets what they leave unsaid: `merge.always`, `spec.parallel`, `base.fix`, `launch.pull` and `logs.dir`. The Architecture review itself never watches CI, so a Base fix can only happen in the dispatched run. The one difference is that it sends no [Run notification](#run-notifications) of its own, whatever `email.always` says: the Architect run sends [the one](#one-run-notification). -The dispatched run's ending is the Architect run's: its exit code, its pull request's URL alone on stdout, and its last line on stderr, `PR is ready for review` or `PR is merged`. If it fails, the Architect run fails as that Failed run or Failed spec run does, with the cause and the session log on stderr and the pull request's URL on stdout if it left one. The plan stays `ready-for-agent`, for `thirdshift ` to take up again. `parallel ` on a plan that is a single Ticket fails the same way as it does for `thirdshift ` on an issue that isn't a Spec: `parallel is only for a Spec, and # has no sub-issues`, exit `1`, before any implementing, with the plan left ready to run without it. +The dispatched run's ending is the Architect run's: its exit code, its pull request's URL alone on stdout, and its last line on stderr, `PR is ready for review` or `PR is merged`. If it fails, the Architect run fails as that Failed run or Failed spec run does, with the cause and the session log on stderr and the pull request's URL on stdout if it left one. The plan stays open, `ready-for-agent` and `architect-plan`, for `thirdshift ` to take up again: no later Architect run [retries it](#one-architect-plan-at-a-time). `parallel ` on a plan that is a single Ticket fails the same way as it does for `thirdshift ` on an issue that isn't a Spec: `parallel is only for a Spec, and # has no sub-issues`, exit `1`, before any implementing, with the plan left ready to run without it. -With `--plan-only`, it instead exits `0` with the plan's URL alone on stdout, and `plan is ready for an agent` as stderr's last line. Read or edit the plan, then run it with `thirdshift `. +With `--plan-only`, it instead exits `0` with the plan's URL alone on stdout, and `plan is ready for an agent` as stderr's last line. Read or edit the plan, then run it with `thirdshift `. The plan is labelled `architect-plan` here too, so until it is finished or closed the next Architect run is [skipped](#one-architect-plan-at-a-time). Progress lines on stderr say when the review starts, which plan it reported, when the labels are swapped, and when the plan is dispatched: `dispatching the plan , as thirdshift would`. The dispatched run's own progress lines follow. With no Strong candidate, the last line says which issue the Architect run ended on instead. -A review session that fails or is interrupted, a final message without one of the lines the prompt asks for, or a plan that fails a check ends the Architect run as a failure: exit `1`, nothing on stdout, the cause on stderr and then the path of the session log. Nothing is dispatched and no label is changed, so a plan the review did publish stays `needs-triage` for you to finish or close. A review that finds no deepening opportunity at all has no issue to name, so it ends without one of those lines and the Architect run fails this way too; its session log says what it looked at. +A review session that fails or is interrupted, a final message without one of the lines the prompt asks for, or a plan that fails a check ends the Architect run as a failure: exit `1`, nothing on stdout, the cause on stderr and then the path of the session log. Nothing is dispatched and no label is changed, so a plan the review did publish stays `needs-triage`, without `architect-plan`, for you to finish or close, and doesn't skip the next Architect run. A review that finds no deepening opportunity at all has no issue to name, so it ends without one of those lines and the Architect run fails this way too; its session log says what it looked at. ### One Run notification -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](#one-at-a-time) included, 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. +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](#one-at-a-time) 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), 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, `an Architect run is already running on /`, in place of the `Review:` line, then the hostname and how long it took. +- **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), 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>`. ### No Strong candidate @@ -458,6 +458,22 @@ thirdshift holds the rule with an operating-system lock on a file under `~/.thir 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. +### One Architect plan at a time + +An Architect run is also skipped while an earlier **Architect plan** is still open, so a new refactor is never planned while the last one is unfinished: its pull request waiting for review, its run failed, or its plan published with `--plan-only` and not started. + +thirdshift marks every Architect plan with the label `architect-plan`, in step 6: the Spec, or the standalone Ticket, never a Spec's Tickets. Once the one-at-a-time lock is its own, and before the launch pull, the worktree and the Architecture review, an Architect run lists the repository's open issues labelled `architect-plan`. If there are any, it is skipped: + +- **stdout** carries each open Architect plan's URL, one a line, newest first. +- **stderr** carries one progress line naming each of them with the command that picks it up, several joined by `; `: `Architect plan #<n> "<title>" is still open: pick it up with thirdshift <plan URL>`. +- **Exit code** `0`: skipped is not a failure. No agent session starts, and nothing else is done. + +It applies to every Architect run, with or without `--plan-only`, and no flag overrides it. The rule is released by finishing the Architect plan, since merging its work closes the issue, by closing the issue, or by removing its `architect-plan` label. The check reads GitHub, so unlike the lock it holds across machines. + +An Architect run never retries or dispatches an existing Architect plan. One whose dispatched run failed stays open, and picking it up is yours to do, with `thirdshift <plan URL>`, which continues whatever branch and pull request the failed run left. What isn't labelled `architect-plan` never counts: an idea issue a review filed with [no Strong candidate](#no-strong-candidate), or a plan that a failed or interrupted review left `needs-triage`. + +The check comes after the lock so that the plan of an Architect run that is still running, labelled already while its Spec run or Run goes on, is reported as [already running](#one-at-a-time), not as an unfinished plan. A skipped run asked for a [Run notification](#one-run-notification) sends its one, with the open Architect plans and their commands in its `Skipped:` line. + ## 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 253c1abd0..8d3cf450b 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -1,13 +1,14 @@ //! An Architect run up to its plan: an Architecture review of the Base //! branch, the one its command named or else the one checked out in the //! Launch directory, from the Launch directory with no Issue URL, then the checks on -//! the plan it published and the label swap that marks the plan ready, for -//! the command to stop at or to dispatch. A review that found no Strong -//! candidate published no plan, and the Architect run ends on the issue it +//! the plan it published and the label change that marks the plan ready and +//! makes it the Architect plan, for the command to stop at or to 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. +//! is skipped, before any review. So is one that finds an Architect plan +//! still open on the repository: it never retries or dispatches that plan. use std::fmt; use std::fs::{self, File, TryLockError}; @@ -35,6 +36,15 @@ use crate::worktree::ReviewWorktree; /// name. const REVIEW: &str = "architecture-review"; +/// The label thirdshift marks an Architect plan with, which a later Architect +/// run finds an open one by. +const ARCHITECT_PLAN_LABEL: &str = "architect-plan"; + +/// The description the `architect-plan` label is added to the repository +/// with if the repository lacks it. +const ARCHITECT_PLAN_DESCRIPTION: &str = + "An Architect plan: the Spec or Ticket an Architecture review published"; + /// How an Architect run ended, short of a failure and before any dispatch. /// Its `Display` is the line that says how it ended. #[derive(Debug)] @@ -54,6 +64,20 @@ impl Outcome { Self::Reviewed(reviewed) => reviewed.review(), } } + + /// 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. + pub fn urls(&self) -> Vec<&str> { + match self { + Self::Skipped(Skipped::AlreadyRunning(_)) => Vec::new(), + Self::Skipped(Skipped::OpenPlans(plans)) => { + plans.iter().map(|(plan, _)| plan.url.as_str()).collect() + } + Self::Reviewed(reviewed) => vec![reviewed.url()], + } + } } impl fmt::Display for Outcome { @@ -72,6 +96,9 @@ 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), + /// These Architect plans, each with its title, are still open on this + /// repository: at least one. + OpenPlans(Vec<(IssueUrl, String)>), } impl fmt::Display for Skipped { @@ -80,6 +107,18 @@ impl fmt::Display for Skipped { Self::AlreadyRunning(repo) => { write!(f, "an Architect run is already running on {}", repo.slug()) } + Self::OpenPlans(plans) => { + let still_open: Vec<String> = plans + .iter() + .map(|(plan, title)| { + format!( + "Architect plan #{} \"{title}\" is still open: pick it up with thirdshift {}", + plan.number, plan.url + ) + }) + .collect(); + f.write_str(&still_open.join("; ")) + } } } } @@ -153,7 +192,10 @@ impl fmt::Display for Reviewed { /// 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. +/// one Architect run until it exits, through whatever it dispatches. 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. pub fn run( focus: Option<&str>, base: Option<&str>, @@ -182,6 +224,10 @@ pub fn run( // 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))); + } if launch_pull { run::pull_base_branch(&launch, checked_out.as_deref(), &base); } @@ -319,8 +365,9 @@ fn conclude( } /// Mark `plan` ready: check it, then swap its `needs-triage` for -/// `ready-for-agent`, in one request. Fails, changing no label, if the plan -/// fails its checks. +/// `ready-for-agent` and label it `architect-plan`, in one request, having +/// added that label to the repository if it lacks it. Fails, changing no +/// label, if the plan fails its checks. fn mark_plan_ready(plan: &IssueUrl, origin: &str, started: DateTime<Utc>) -> Result<()> { progress::step(format_args!( "the Architecture review published the plan {}", @@ -331,15 +378,21 @@ fn mark_plan_ready(plan: &IssueUrl, origin: &str, started: DateTime<Utc>) -> Res bail!("interrupted"); } progress::step(format_args!( - "marking the plan ready: swapping {NEEDS_TRIAGE} for {READY_FOR_AGENT} on #{}", + "marking the plan ready: swapping {NEEDS_TRIAGE} for {READY_FOR_AGENT} and adding {ARCHITECT_PLAN_LABEL} on #{}", plan.number )); + github::ensure_labels( + &plan.repo_slug(), + &[(ARCHITECT_PLAN_LABEL, ARCHITECT_PLAN_DESCRIPTION)], + )?; + let added = [READY_FOR_AGENT, ARCHITECT_PLAN_LABEL]; + // GitHub's label names are case-insensitive. let mut labels: Vec<&str> = kept .iter() .map(String::as_str) - .filter(|&label| label != READY_FOR_AGENT) + .filter(|kept| !added.iter().any(|added| added.eq_ignore_ascii_case(kept))) .collect(); - labels.push(READY_FOR_AGENT); + labels.extend(added); github::set_labels(plan, &labels) } diff --git a/src/base_fix.rs b/src/base_fix.rs index 49d1223b5..a27773f7f 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -356,7 +356,7 @@ fn open_fix_covering( inherited: &[Check], ) -> Result<Option<IssueUrl>> { let before_checks = issue_title(base, ""); - let covering = github::open_issues_labelled(issue, BASE_FIX_LABEL)? + let covering = github::open_issues_labelled(&issue.repo_slug(), BASE_FIX_LABEL)? .into_iter() .filter(|(open, title)| { open.number != issue.number diff --git a/src/github.rs b/src/github.rs index df941ea13..5436d3f56 100644 --- a/src/github.rs +++ b/src/github.rs @@ -195,15 +195,16 @@ pub fn close_issue(issue: &IssueUrl, comment: &str) -> Result<()> { ]) } -/// Every open issue labelled `label` in the repository of `issue`, with its -/// title. 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(issue: &IssueUrl, label: &str) -> Result<Vec<(IssueUrl, 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)>> { let json = gh_json(&[ "issue", "list", "--repo", - &issue.repo_slug(), + repo, "--label", label, "--state", @@ -227,19 +228,11 @@ pub fn open_issues_labelled(issue: &IssueUrl, label: &str) -> Result<Vec<(IssueU .collect() } -/// Open an issue titled `title`, with `body` and `labels`, in the repository -/// of `issue`, and return it. Each label is first added to the repository, -/// with its description, if the repository lacks it: `gh` refuses a label it -/// doesn't know. -pub fn create_issue( - issue: &IssueUrl, - title: &str, - body: &str, - labels: &[(&str, &str)], -) -> Result<IssueUrl> { - let repo = issue.repo_slug(); +/// Add each of `labels` to the repository `repo`, an `owner/repo`, with its +/// description, unless the repository has it already. +pub fn ensure_labels(repo: &str, labels: &[(&str, &str)]) -> Result<()> { let known = gh_json(&[ - "label", "list", "--repo", &repo, "--json", "name", "--limit", "1000", + "label", "list", "--repo", repo, "--json", "name", "--limit", "1000", ])?; let known: Vec<&str> = known .as_array() @@ -255,12 +248,27 @@ pub fn create_issue( "create", name, "--repo", - &repo, + repo, "--description", description, ])?; } } + Ok(()) +} + +/// Open an issue titled `title`, with `body` and `labels`, in the repository +/// of `issue`, and return it. Each label is first added to the repository, +/// with its description, if the repository lacks it: `gh` refuses a label it +/// doesn't know. +pub fn create_issue( + issue: &IssueUrl, + title: &str, + body: &str, + labels: &[(&str, &str)], +) -> Result<IssueUrl> { + let repo = issue.repo_slug(); + ensure_labels(&repo, labels)?; let names: Vec<&str> = labels.iter().map(|(name, _)| *name).collect(); let url = gh_stdout(&[ "issue", diff --git a/src/main.rs b/src/main.rs index 8470c5d05..d0701b98f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -101,8 +101,10 @@ Architecture review, an agent session in its own worktree at the Base branch's h looks for deepening opportunities and publishes the top one as a plan: a Spec with Tickets, or a single Ticket. thirdshift then checks that the plan is open, new and not labelled ready-for-human, needs-info or wontfix, swaps its needs-triage label for ready-for-agent, -and dispatches it as thirdshift <Issue URL> would: a Spec run on a Spec, a Run on a single -Ticket. The Architect run ends as that run does, with its exit code and its PR's URL. +labels it architect-plan, creating the label if the repository lacks it, and dispatches it +as thirdshift <Issue URL> would: a Spec run on a Spec, a Run on a single Ticket. A Spec's +Tickets are never labelled architect-plan. The Architect run ends as that run does, with +its exit code and its PR's URL. merge, --no-merge, base-fix, --no-base-fix and parallel <n> apply to that run, as do the User config's defaults; parallel <n> fails it if the plan is a single Ticket. The review itself watches no CI, so only that run can start a Base fix. With --plan-only, the @@ -140,6 +142,13 @@ skipped: it prints an Architect run is already running on <owner>/<repo>, does n 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 +each open Architect plan, prints its URL on stdout, gives the command that picks it up, +thirdshift <plan URL>, and exits 0. No flag overrides this: finish or close the Architect +plan, or remove its label. An Architect run never retries or dispatches an existing +Architect plan, so one whose run failed stays open until you pick it up. + --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 @@ -271,9 +280,10 @@ fn main() -> ExitCode { /// to dispatch, puts the URL of the issue it ended on on stdout: the plan, /// the idea issue the review filed, or the 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, as another on its repository -/// is still running, says so on stderr, puts nothing on stdout, and is no -/// failure. If asked, by the command or the User config, it sends one Run +/// 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. fn architect(args: ArchitectArgs) -> ExitCode { @@ -320,9 +330,7 @@ fn architect(args: ArchitectArgs) -> ExitCode { (Ok(outcome), None) => { // Also on stderr, so the outcome shows even when stdout is captured. progress::step(format_args!("{outcome}")); - if let Outcome::Reviewed(reviewed) = outcome { - print_url(reviewed.url()); - } + outcome.urls().into_iter().for_each(print_url); ExitCode::SUCCESS } (Err(failed), None) => report(failed), diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 299e87e38..a788a74f7 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -1,7 +1,8 @@ //! Architect runs: `thirdshift architect` runs the Architecture review in its //! own worktree, detached at the Base branch's head on origin, then checks -//! the plan the review published and swaps its `needs-triage` for -//! `ready-for-agent`. With `--plan-only` it prints the plan's URL and stops. +//! the plan the review published, swaps its `needs-triage` for +//! `ready-for-agent` and labels it `architect-plan`. With `--plan-only` it +//! prints the plan's URL and stops. //! Without, it dispatches the plan as `thirdshift <plan URL>` would, a Spec //! run or a Run, and ends as that does. A review with no Strong candidate //! has no plan: the Architect run prints the URL of the idea issue it filed, @@ -12,6 +13,8 @@ //! 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. mod support; @@ -26,6 +29,9 @@ const PLAN_URL: &str = "https://github.com/acme/widgets/issues/8"; /// The first pull request opened on the fake GitHub. const PR_URL: &str = "https://github.com/acme/widgets/pull/1"; +/// The label thirdshift marks an Architect plan with. +const ARCHITECT_PLAN: &str = "architect-plan"; + const NO_FINAL_LINE: &str = "the Architecture review ended without the final line its prompt asks for"; @@ -215,7 +221,10 @@ fn plan_only_marks_the_published_ticket_ready_and_prints_its_url() { assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); assert_eq!(result.stdout, format!("{PLAN_URL}\n")); - assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + assert_eq!( + scenario.issue_labels(8), + ["ready-for-agent", ARCHITECT_PLAN] + ); assert_nothing_left_behind(&scenario); } @@ -236,11 +245,59 @@ printf 'Architecture review plan: %s\n' "$spec" > "$FAKE_CLAUDE_FINAL_MESSAGE" assert_eq!(result.stdout, format!("{PLAN_URL}\n")); assert_eq!( scenario.issue_labels(8), - ["architecture", "ready-for-agent"] + ["architecture", "ready-for-agent", ARCHITECT_PLAN] ); assert_eq!(scenario.issue_labels(9), ["ready-for-agent"]); } +/// The repository's labels on the fake GitHub. +fn repo_labels(scenario: &Scenario) -> Vec<String> { + let labels = scenario.gh_state()["repo_labels"].clone(); + serde_json::from_value(labels).unwrap() +} + +#[test] +fn the_architect_plan_label_is_created_when_the_repository_lacks_it() { + let scenario = scenario(); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + assert!(!repo_labels(&scenario).contains(&ARCHITECT_PLAN.to_string())); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!( + repo_labels(&scenario), + [ + "needs-triage", + "ready-for-agent", + "architecture", + ARCHITECT_PLAN + ] + ); + let created = scenario.gh_calls_of("label", "create"); + assert_eq!(created.len(), 1, "{created:?}"); + assert_eq!(created[0][2], ARCHITECT_PLAN); +} + +#[test] +fn a_repository_that_has_the_architect_plan_label_keeps_it_as_it_is() { + let scenario = scenario(); + let mut gh = scenario.gh_state(); + gh["repo_labels"] = serde_json::json!(["needs-triage", "Architect-Plan"]); + scenario.write_gh_state(&gh); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(repo_labels(&scenario), ["needs-triage", "Architect-Plan"]); + assert!(scenario.gh_calls_of("label", "create").is_empty()); + assert_eq!( + scenario.issue_labels(8), + ["ready-for-agent", ARCHITECT_PLAN] + ); +} + #[test] fn the_session_prompt_names_the_factory_skills_the_base_branch_and_the_final_line() { let scenario = scenario(); @@ -351,7 +408,7 @@ fn progress_lines_cover_the_review_starting_the_plan_it_reported_and_the_label_s .to_string(), "thirdshift: architecture-review: session started\n".to_string(), format!("thirdshift: the Architecture review published the plan {PLAN_URL}\n"), - "thirdshift: marking the plan ready: swapping needs-triage for ready-for-agent on #8\n" + "thirdshift: marking the plan ready: swapping needs-triage for ready-for-agent and adding architect-plan on #8\n" .to_string(), ] { let Some(at) = rest.find(&line) else { @@ -641,7 +698,10 @@ fn a_single_ticket_plan_starts_a_run_on_it_and_ends_as_that_run_does() { let pr = pr_from(&scenario, "issue-8"); assert_ended_with_pr(&result, &pr, "ready for review"); assert_eq!(pr["url"], PR_URL); - assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + assert_eq!( + scenario.issue_labels(8), + ["ready-for-agent", ARCHITECT_PLAN] + ); let calls = scenario.claude_calls(); assert_eq!(calls.len(), 2, "sessions: {calls:?}"); let prompt = calls[1]["prompt"].as_str().unwrap(); @@ -670,7 +730,10 @@ fn a_plan_with_tickets_starts_a_spec_run_on_it_and_ends_as_that_spec_run_does() assert_eq!(spec_pr["base"], "main"); assert_eq!(spec_pr["state"], "OPEN"); assert_eq!(spec_pr["isDraft"], false); - assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + assert_eq!( + scenario.issue_labels(8), + ["ready-for-agent", ARCHITECT_PLAN] + ); let dispatch = format!("dispatching the plan {PLAN_URL}, as thirdshift {PLAN_URL} would\n"); let dispatched = result.stderr.find(&dispatch); let started = result.stderr.find("thirdshift: starting #9\n"); @@ -689,6 +752,7 @@ fn a_plan_with_tickets_starts_a_spec_run_on_it_and_ends_as_that_spec_run_does() assert_eq!(pr["base"], "issue-8"); assert_eq!(pr["state"], "MERGED"); assert_eq!(gh["issues"][ticket.to_string()], "CLOSED"); + assert_eq!(scenario.issue_labels(ticket), ["ready-for-agent"]); assert_eq!( scenario.origin_file("issue-8", &format!("issue-{ticket}.txt")), Some(format!("{ticket}\n")) @@ -759,7 +823,10 @@ fn parallel_on_a_single_ticket_plan_fails_as_it_does_for_an_issue_that_is_not_a_ result.stderr ); assert_eq!(scenario.claude_calls().len(), 1, "a Run was started"); - assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + assert_eq!( + scenario.issue_labels(8), + ["ready-for-agent", ARCHITECT_PLAN] + ); assert!(scenario.origin_log("issue-8").is_none()); assert_nothing_left_behind(&scenario); } @@ -962,7 +1029,10 @@ fn a_dispatched_run_that_fails_fails_the_architect_run_as_a_failed_run_does() { "stderr: {}", result.stderr ); - assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + assert_eq!( + scenario.issue_labels(8), + ["ready-for-agent", ARCHITECT_PLAN] + ); } #[test] @@ -974,7 +1044,7 @@ fn progress_lines_show_the_dispatch_after_the_label_swap_and_before_the_run() { assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); let mut rest = result.stderr.as_str(); for line in [ - "thirdshift: marking the plan ready: swapping needs-triage for ready-for-agent on #8\n" + "thirdshift: marking the plan ready: swapping needs-triage for ready-for-agent and adding architect-plan on #8\n" .to_string(), format!("thirdshift: dispatching the plan {PLAN_URL}, as thirdshift {PLAN_URL} would\n"), "thirdshift: implement: session started\n".to_string(), @@ -1686,9 +1756,16 @@ fn a_second_architect_run_is_skipped_while_the_firsts_dispatched_run_or_spec_run ), ); let first = scenario.run_until(&["architect"], &[], "started"); + assert!( + scenario + .issue_labels(8) + .contains(&ARCHITECT_PLAN.to_string()) + ); let second = scenario.run(&["architect"]); + // Never as one whose Architect plan is still open: the lock is tried + // first, so the first run's own, labelled by now, is not reported. assert_skipped_as_already_running(&second); release(&scenario); let first = first.finish(); @@ -1707,6 +1784,9 @@ fn once_the_first_architect_run_has_ended_a_new_one_runs() { scenario.agent_does(AGENT_PUBLISHES_A_TICKET); let first = scenario.run(&["architect", "--plan-only"]); assert_eq!(first.code, Some(0), "stderr: {}", first.stderr); + // Its plan is done with, so only a first run still going could skip the + // next. + scenario.issue_is(8, "CLOSED"); let second = scenario.run(&["architect", "--plan-only"]); @@ -1749,10 +1829,20 @@ fn an_architect_run_on_a_different_repository_at_the_same_time_is_not_skipped() ); let first = scenario.run_until(&["architect", "--plan-only"], &[], "started"); // The first Architect run has read its repository, acme/widgets, by now: - // the next one from the Launch directory is on another. + // the next one from the Launch directory is on another, with a fake + // GitHub of its own. scenario.set_origin_url("https://github.com/acme/gadgets.git"); + let gadgets = scenario.path("gh-state-gadgets.json"); + fs::write( + &gadgets, + r#"{"repo": "acme/gadgets", "issues": {}, "prs": []}"#, + ) + .unwrap(); - let second = scenario.run(&["architect", "--plan-only"]); + let second = scenario.run_with_env( + &["architect", "--plan-only"], + &[("FAKE_GH_STATE", gadgets.to_str().unwrap())], + ); assert_eq!(second.code, Some(0), "stderr: {}", second.stderr); assert_eq!(second.stdout, format!("{gadgets_issue}\n")); @@ -1818,3 +1908,177 @@ fn the_notifications_checks_come_before_the_skip_so_a_skip_never_goes_unnotified release(&scenario); first.finish(); } + +/// Make issue `number` an open Architect plan titled `title`, as thirdshift +/// left the plan of an earlier Architect run. +fn open_architect_plan(scenario: &Scenario, number: u32, title: &str) { + scenario.issue_is(number, "OPEN"); + scenario.issue_titled(number, title); + scenario.issue_labelled(number, &["ready-for-agent", ARCHITECT_PLAN]); +} + +/// What a skipped Architect run says of the open Architect plan `number`, +/// titled `title`, on the scenario's repository. +fn still_open(scenario: &Scenario, number: u32, title: &str) -> String { + format!( + "Architect plan #{number} \"{title}\" is still open: pick it up with thirdshift {}", + scenario.issue_url(number) + ) +} + +#[test] +fn an_open_architect_plan_skips_the_architect_run_before_any_review_whatever_its_flags() { + for args in [ + vec!["architect"], + vec!["architect", "--plan-only"], + vec!["architect", "the Spec run", "merge", "base", "main"], + ] { + let scenario = scenario(); + scenario.user_config_is("[launch]\npull = true\n"); + open_architect_plan(&scenario, 7, "Deepen the session module"); + scenario.origin_has_commit("main", "upstream.txt", "upstream\n", "Upstream work"); + let launch_main = scenario.launch_git(&["rev-parse", "refs/heads/main"]); + let github = scenario.gh_state(); + + let result = scenario.run(&args); + + assert_eq!(result.code, Some(0), "{args:?}: stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{}\n", scenario.issue_url(7))); + assert_eq!( + result.stderr, + format!( + "thirdshift: {}\n", + still_open(&scenario, 7, "Deepen the session module") + ) + ); + assert!(scenario.claude_calls().is_empty(), "claude was started"); + assert_eq!( + scenario.launch_git(&["rev-parse", "refs/heads/main"]), + launch_main, + "the skipped run did a launch pull" + ); + assert_eq!(scenario.gh_state(), github); + assert_nothing_left_behind(&scenario); + } +} + +#[test] +fn a_skipped_architect_run_names_each_open_architect_plan_and_prints_its_url() { + let scenario = scenario(); + open_architect_plan(&scenario, 5, "Deepen the worktree module"); + open_architect_plan(&scenario, 7, "Deepen the session module"); + + let result = scenario.run(&["architect"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!( + result.stdout, + format!("{}\n{}\n", scenario.issue_url(7), scenario.issue_url(5)) + ); + assert_eq!( + result.stderr, + format!( + "thirdshift: {}; {}\n", + still_open(&scenario, 7, "Deepen the session module"), + still_open(&scenario, 5, "Deepen the worktree module") + ) + ); + assert!(scenario.claude_calls().is_empty(), "claude was started"); +} + +#[test] +fn an_architect_plan_that_is_closed_or_no_longer_labelled_lets_the_architect_run_go_ahead() { + for lift_the_rule in [ + (|scenario| scenario.issue_is(7, "CLOSED")) as fn(&Scenario), + |scenario| scenario.issue_labelled(7, &["ready-for-agent"]), + ] { + let scenario = scenario(); + open_architect_plan(&scenario, 7, "Deepen the session module"); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + lift_the_rule(&scenario); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PLAN_URL}\n")); + assert_eq!(scenario.claude_calls().len(), 1); + } +} + +#[test] +fn an_open_idea_issue_or_a_plan_left_needing_triage_does_not_skip_the_architect_run() { + // #7 is the idea issue an earlier review filed, or the plan one that + // failed left half-published: neither was ever labelled an Architect plan. + let scenario = scenario(); + scenario.issue_labelled(7, &["needs-triage", "architecture"]); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PLAN_URL}\n")); + assert_eq!(scenario.issue_labels(7), ["needs-triage", "architecture"]); +} + +#[test] +fn a_review_that_failed_after_publishing_its_plan_does_not_skip_the_next_architect_run() { + let scenario = scenario(); + scenario.agent_does_in_session(1, &publishes_a_ticket_then("exit 3")); + scenario.agent_does_in_session(2, AGENT_PUBLISHES_A_TICKET); + let first = scenario.run(&["architect", "--plan-only"]); + assert_eq!(first.code, Some(1), "stderr: {}", first.stderr); + + let second = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(second.code, Some(0), "stderr: {}", second.stderr); + assert_eq!(second.stdout, format!("{}\n", scenario.issue_url(9))); + assert_eq!(scenario.issue_labels(8), ["needs-triage"]); +} + +#[test] +fn a_plan_whose_dispatched_run_failed_stays_open_and_the_next_architect_run_never_retries_it() { + let scenario = single_ticket_plan(); + scenario.agent_does_for(8, "exit 3"); + let first = scenario.run(&["architect"]); + assert_eq!(first.code, Some(1), "stderr: {}", first.stderr); + let sessions = scenario.claude_calls().len(); + + let second = scenario.run(&["architect"]); + + assert_eq!(second.code, Some(0), "stderr: {}", second.stderr); + assert_eq!(second.stdout, format!("{PLAN_URL}\n")); + assert_eq!( + second.stderr, + format!( + "thirdshift: {}\n", + still_open(&scenario, 8, "Deepen the session module") + ) + ); + assert_eq!(scenario.claude_calls().len(), sessions, "a session started"); +} + +#[test] +fn an_architect_run_skipped_for_open_plans_sends_one_notification_naming_each_and_its_command() { + let scenario = scenario(); + open_architect_plan(&scenario, 5, "Deepen the worktree module"); + open_architect_plan(&scenario, 7, "Deepen the session module"); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--plan-only", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!(subject, "[thirdshift] acme/widgets Architect run: skipped"); + assert!( + text.starts_with(&format!( + "Skipped: {}; {}\nHost: ", + still_open(&scenario, 7, "Deepen the session module"), + still_open(&scenario, 5, "Deepen the worktree module") + )), + "{text}" + ); +} diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 0d9a7236d..6a23bb9e8 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -70,7 +70,8 @@ fn help_does_not_mention_the_flag_aliases() { } #[test] -fn help_documents_architect_its_focus_the_dispatch_plan_only_and_one_architect_run_at_a_time() { +fn help_documents_architect_its_focus_the_dispatch_plan_only_and_when_an_architect_run_is_skipped() +{ let scenario = Scenario::new(); let help = scenario.run(&["help"]).stdout; @@ -89,6 +90,12 @@ fn help_documents_architect_its_focus_the_dispatch_plan_only_and_one_architect_r "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", "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", + "An Architect run that finds an open issue labelled architect-plan is skipped too, before any review, with or without --plan-only", + "prints its URL on stdout, gives the command that picks it up, thirdshift <plan URL>, and exits 0", + "No flag overrides this: finish or close the Architect plan, or remove its label", + "An Architect run never retries or dispatches an existing Architect plan", "--email, --email <address> and --no-email ask an Architect run for its Run notification", "It sends one for the whole Architect run, however it ends", "The run the plan is dispatched as sends none of its own",