diff --git a/README.md b/README.md index b38673811..441cdb084 100644 --- a/README.md +++ b/README.md @@ -167,7 +167,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). `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. +`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), the URL of each Architect plan that is [still open](#one-architect-plan-at-a-time), and the URL of each [Architect idea waiting for triage](#an-architect-idea-waits-for-triage). `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. @@ -489,9 +489,9 @@ 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, 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. +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), and if not, when an [Architect idea waits for triage](#an-architect-idea-waits-for-triage). 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. +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, which thirdshift labels an **Architect idea**, 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`, the plan is labelled `architect-plan`, the mark a later Architect run finds an open **Architect plan** by, and its other labels are kept. thirdshift adds that label, never the agent, and first creates it if the repository has none. A Spec's Tickets are left as the review labelled them, without `architect-plan`. @@ -510,15 +510,27 @@ 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 `Command 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 /`, 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 `Command 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>`, 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>`, or each Architect idea waiting for triage: `Architect idea #<n> "<title>" is waiting for triage: <Issue URL>`. ### No Strong candidate -Only a Strong top recommendation becomes a plan. When the review's top recommendation is Worth exploring or Speculative, it publishes no plan, and the Architect run ends in one of two ways, both a success, with or without `--plan-only`: exit `0`, with one issue's URL alone on stdout. thirdshift changes no label on that issue and dispatches nothing: there is nothing to run. +Only a Strong top recommendation becomes a plan. When the review's top recommendation is Worth exploring or Speculative, it publishes no plan, and the Architect run ends in one of two ways, both a success, with or without `--plan-only`: exit `0`, with one issue's URL alone on stdout. thirdshift labels that issue an **Architect idea** and dispatches nothing: there is nothing to run. - **An idea issue.** The review files its top recommendation as one issue labelled `needs-triage`, for the **Day shift** to flesh out, and ends its final message with `Architecture review idea: <Issue URL>`. stdout carries the idea issue's URL, and stderr's last line is `no Strong candidate: the Architecture review filed the idea <url>`. - **Already filed.** An open issue already covers that recommendation, so the review files nothing and ends its final message with `Architecture review already filed: <Issue URL>`. stdout carries that issue's URL, and stderr's last line is `no Strong candidate: <url> already covers the Architecture review's top recommendation, so it filed nothing`. +Either way, thirdshift then puts `architect-idea` and `needs-triage` on that issue, in one request that keeps its other labels, with a progress line saying so: `labelling #<n> an Architect idea: adding needs-triage and architect-idea`. thirdshift adds `architect-idea`, never the agent, and first creates it if the repository has none, as it does `architect-plan`. `needs-triage` goes back on an issue that already covered the idea even if it had been triaged, `ready-for-human` say, since the factory again takes it for the best next move. If thirdshift can't label the issue, the Architect run fails as a failed review does, with the cause on stderr, `could not label the Architect idea #<n>: <why>`, and in its [Run notification](#one-run-notification). + +### An Architect idea waits for triage + +An Architect idea means the factory has run out of Strong ideas, so while any Architect idea is open and still labelled `needs-triage`, every later Architect run on the repository is skipped, before any review, instead of filing the next weaker idea or landing on the same issue again. Both labels count in any case, as every label rule does. The check comes after the [lock](#one-at-a-time) and the [open Architect plan](#one-architect-plan-at-a-time) check, so with both an open plan and a waiting idea, the skip reports the plan: + +- **stdout** carries each waiting Architect idea's URL, one a line, newest first. +- **stderr** carries one progress line naming each of them as waiting for triage, several joined by `; `: `Architect idea #<n> "<title>" is waiting for triage: <Issue URL>`. +- **Exit code** `0`: skipped is not a failure. No agent session starts, and nothing else is done. + +Any triage decision releases the pause: take `needs-triage` off, whatever replaces it (`ready-for-agent`, `ready-for-human`, `needs-info`, `wontfix` or nothing), or close the issue. The issue keeps `architect-idea`, which only counts alongside `needs-triage`. + ### One at a time 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. @@ -541,7 +553,7 @@ thirdshift marks every Architect plan with the label `architect-plan`, in step 6 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`. +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 here: a plan that a failed or interrupted review left `needs-triage`, or an [Architect idea](#no-strong-candidate), which [pauses Architect runs](#an-architect-idea-waits-for-triage) by its own rule, only while it waits for 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. @@ -569,7 +581,7 @@ 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, 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. +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, when an [Architect plan is still open](#one-architect-plan-at-a-time), or when an [Architect idea waits for triage](#an-architect-idea-waits-for-triage): 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. diff --git a/src/architect.rs b/src/architect.rs index 7f9be2df4..09659a21d 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -4,18 +4,20 @@ //! checks on the Architect plan it published and the label change that marks //! it ready and labels it `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 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. +//! the Architect run ends on the issue it named instead, labelled an +//! Architect idea: the idea issue it filed for its top recommendation, or the +//! open issue that already covers 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. And so is +//! one that finds an Architect idea there waiting for triage: the factory has +//! run out of Strong ideas until the Day shift decides on it. use std::fmt; use std::path::{Path, PathBuf}; -use anyhow::{Result, anyhow, bail}; +use anyhow::{Context, Result, anyhow, bail}; use chrono::{DateTime, Utc}; use crate::command_log; @@ -43,6 +45,13 @@ const ARCHITECT_PLAN: Label = Label::new( "An Architect plan: the Spec or Ticket an Architecture review published", ); +/// The label thirdshift marks an Architect idea with, which, with +/// `needs-triage`, a later Architect run finds one waiting for triage by. +const ARCHITECT_IDEA: Label = Label::new( + "architect-idea", + "An Architect idea: the issue an Architecture review with no Strong candidate ended on", +); + /// 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)] @@ -65,13 +74,14 @@ 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, or a Pickup run, is still running. + /// review ended on, or each open Architect plan, or each Architect idea + /// waiting for triage, it was skipped for. None 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(), - Self::Skipped(Skipped::OpenPlans(plans)) => { - plans.iter().map(|(plan, _)| plan.url.as_str()).collect() + Self::Skipped(Skipped::OpenPlans(issues) | Skipped::IdeasWaiting(issues)) => { + issues.iter().map(|(issue, _)| issue.url.as_str()).collect() } Self::Reviewed(reviewed) => vec![reviewed.url()], } @@ -97,28 +107,44 @@ pub enum Skipped { /// These Architect plans, each with its title, are still open on this /// repository: at least one. OpenPlans(Vec<(IssueUrl, String)>), + /// These Architect ideas, each with its title, are open on this + /// repository and still labelled `needs-triage`: at least one. + IdeasWaiting(Vec<(IssueUrl, String)>), } impl fmt::Display for Skipped { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { Self::AlreadyRunning(running) => running.fmt(f), - 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("; ")) - } + Self::OpenPlans(plans) => each(f, plans, |plan, title| { + format!( + "Architect plan #{} \"{title}\" is still open: pick it up with thirdshift {}", + plan.number, plan.url + ) + }), + Self::IdeasWaiting(ideas) => each(f, ideas, |idea, title| { + format!( + "Architect idea #{} \"{title}\" is waiting for triage: {}", + idea.number, idea.url + ) + }), } } } +/// Write what `says` of each of `issues`, with its title, joined by `; `. +fn each( + f: &mut fmt::Formatter, + issues: &[(IssueUrl, String)], + says: impl Fn(&IssueUrl, &str) -> String, +) -> fmt::Result { + let said: Vec<String> = issues + .iter() + .map(|(issue, title)| says(issue, title)) + .collect(); + f.write_str(&said.join("; ")) +} + /// How an Architecture review ended, short of a failure, with the issue it /// ended on. Its `Display` is the line that says how it ended, naming that /// issue. @@ -178,9 +204,10 @@ impl fmt::Display for Reviewed { /// the branch the command named, whatever the Launch directory has checked /// out, or without one the branch checked out there. A review that reports /// an idea issue it filed, or the open issue that already covers its top -/// recommendation, instead of a plan, changes no label. With `launch_pull`, -/// the Launch directory's checkout of the Base branch, if that is the branch -/// checked out, is first brought up to date with origin. The review's +/// recommendation, instead of a plan, has that issue labelled an Architect +/// idea. With `launch_pull`, the Launch directory's checkout of the Base +/// branch, if that is the branch checked out, is first brought up to date +/// with origin. The review's /// worktree and the plugin directory are gone when this returns. A failure /// after the plan is published leaves its labels as the review left them. /// @@ -189,8 +216,9 @@ impl fmt::Display for Reviewed { /// 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. Past both, it keeps its Command log under -/// `logs_dir`, where its Session logs go too. +/// for an unfinished one. It is then skipped if the repository has an open +/// Architect idea still labelled `needs-triage`. Past all three, it keeps its +/// Command log under `logs_dir`, where its Session logs go too. pub fn run( focus: Option<&str>, base: Option<&str>, @@ -215,6 +243,15 @@ pub fn run( let open_plans = open_plans.into_iter().map(|plan| (plan.issue, plan.title)); return Ok(Outcome::Skipped(Skipped::OpenPlans(open_plans.collect()))); } + let ideas = github::open_issues_labelled(&repo.slug(), ARCHITECT_IDEA)?; + let waiting: Vec<_> = ideas + .into_iter() + .filter(|idea| idea.labels.has(NEEDS_TRIAGE)) + .map(|idea| (idea.issue, idea.title)) + .collect(); + if !waiting.is_empty() { + return Ok(Outcome::Skipped(Skipped::IdeasWaiting(waiting))); + } command_log::keep(command_log::of_architect_run(logs_dir, &repo)); if launch_pull { run::pull_base_branch(&launch, checked_out.as_deref(), &base); @@ -294,8 +331,9 @@ impl Report { /// End the Architecture review, of the Base branch `base`, on the issue the /// last line of its `final_message` names: a plan is marked ready, and an /// idea issue, or the issue that already covers the top recommendation, is -/// left as it is. Fails if the session had no final message, if its last line -/// is not one the prompt asks for, or if the plan can't be marked ready. +/// labelled an Architect idea. Fails if the session had no final message, if +/// its last line is not one the prompt asks for, or if the plan can't be +/// marked ready or the idea labelled. fn conclude( final_message: Option<&str>, origin: &str, @@ -308,8 +346,14 @@ fn conclude( let base = base.to_string(); Ok(Reviewed::PlanReady { plan, base }) } - Some(Report::Idea(idea)) => Ok(Reviewed::IdeaFiled(idea)), - Some(Report::AlreadyFiled(issue)) => Ok(Reviewed::AlreadyFiled(issue)), + Some(Report::Idea(idea)) => { + label_idea(&idea)?; + Ok(Reviewed::IdeaFiled(idea)) + } + Some(Report::AlreadyFiled(issue)) => { + label_idea(&issue)?; + Ok(Reviewed::AlreadyFiled(issue)) + } None => bail!("the Architecture review ended without the final line its prompt asks for"), } } @@ -335,6 +379,27 @@ fn mark_plan_ready(plan: &IssueUrl, origin: &str, started: DateTime<Utc>) -> Res github::set_labels(plan, &kept.swapped(&[], &[READY_FOR_AGENT, ARCHITECT_PLAN])) } +/// Label `idea` an Architect idea: put `needs-triage` and `architect-idea` +/// on it, in one request that keeps its other labels, having added +/// `architect-idea` to the repository if it lacks it. `needs-triage` goes +/// back on an issue that had been triaged: the factory again takes it for the +/// best next move. +fn label_idea(idea: &IssueUrl) -> Result<()> { + if interrupt::requested() { + bail!("interrupted"); + } + progress::step(format_args!( + "labelling #{} an Architect idea: adding {NEEDS_TRIAGE} and {ARCHITECT_IDEA}", + idea.number + )); + let label = || -> Result<()> { + github::ensure_labels(&idea.repo_slug(), &[ARCHITECT_IDEA])?; + let labels = github::issue_labels(idea)?; + github::set_labels(idea, &labels.swapped(&[], &[NEEDS_TRIAGE, ARCHITECT_IDEA])) + }; + label().with_context(|| format!("could not label the Architect idea #{}", idea.number)) +} + /// The labels `plan` keeps once it is marked ready: all it has but /// `needs-triage`. Fails unless the plan passes its checks: it is in the /// repository at `origin`, open, created since the Architect run `started`, diff --git a/src/main.rs b/src/main.rs index 12fe4f8d3..232776944 100644 --- a/src/main.rs +++ b/src/main.rs @@ -153,8 +153,11 @@ 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 -thirdshift prints that issue's URL instead, changing no label. Its last line says which: the -review filed the idea, or it filed nothing. +thirdshift prints that issue's URL instead. Its last line says which: the review filed the +idea, or it filed nothing. Either way thirdshift labels that issue an Architect idea: +architect-idea and needs-triage, in one request that keeps its other labels, creating +architect-idea if the repository lacks it. needs-triage goes back on an issue that had been +triaged. If the issue can't be labelled, the Architect run fails. A review that fails, is interrupted, or ends without naming one of these issues fails the Architect run and leaves any plan it published labelled needs-triage. One that finds no @@ -174,6 +177,11 @@ thirdshift <plan URL>, and exits 0. No flag overrides this: finish or close the 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. +Past that, an Architect run that finds an Architect idea open and still labelled needs-triage +is skipped as well: the factory has run out of Strong ideas. It prints each such idea's URL on +stdout, names it on stderr as waiting for triage, and exits 0. Any triage decision lets the +next Architect run go ahead: take needs-triage off, whatever replaces it, or close the issue. + 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. diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 609b9d238..de13bd24c 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -13,9 +13,11 @@ //! 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. +//! thirdshift labels the issue such a review ends on an Architect idea, with +//! `architect-idea` and `needs-triage`. //! 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. +//! is still running, while an Architect plan is still open there, or while an +//! Architect idea there waits for triage, is skipped. mod support; @@ -33,6 +35,9 @@ 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"; +/// The label thirdshift marks an Architect idea with. +const ARCHITECT_IDEA: &str = "architect-idea"; + /// The label of a Claimed issue, which the run a plan is dispatched as /// swaps the plan's `ready-for-agent` for. const IN_PROGRESS: &str = "in-progress"; @@ -522,7 +527,7 @@ fn a_session_that_ends_without_a_valid_final_line_fails_the_architect_run() { } #[test] -fn a_review_that_files_an_idea_prints_its_url_and_changes_no_label() { +fn a_review_that_files_an_idea_prints_its_url_and_labels_it_an_architect_idea() { let scenario = scenario(); let idea = scenario.issue_url(8); scenario.agent_does( @@ -540,18 +545,19 @@ printf 'No Strong candidate.\n\nArchitecture review idea: %s\n' "$url" > "$FAKE_ &idea, &format!("no Strong candidate: the Architecture review filed the idea {idea}"), ); - assert_eq!(scenario.issue_labels(8), ["needs-triage"]); + assert_eq!(scenario.issue_labels(8), ["needs-triage", ARCHITECT_IDEA]); } #[test] -fn a_review_whose_idea_is_already_filed_prints_that_issues_url_and_files_and_changes_nothing() { +fn a_review_whose_idea_is_already_filed_prints_that_issues_url_files_nothing_and_labels_it_an_architect_idea() + { let scenario = scenario(); scenario.issue_labelled(7, &["needs-triage"]); let url = scenario.issue_url(7); scenario.agent_does(&ends_with(&format!( "Architecture review already filed: {url}" ))); - let github = scenario.gh_state(); + let mut github = scenario.gh_state(); let result = scenario.run(&["architect", "--plan-only"]); @@ -563,16 +569,111 @@ fn a_review_whose_idea_is_already_filed_prints_that_issues_url_and_files_and_cha "no Strong candidate: {url} already covers the Architecture review's top recommendation, so it filed nothing" ), ); + github["labels"]["7"] = serde_json::json!(["needs-triage", ARCHITECT_IDEA]); + github["repo_labels"] + .as_array_mut() + .unwrap() + .push(ARCHITECT_IDEA.into()); assert_eq!(scenario.gh_state(), github); } +#[test] +fn an_idea_already_filed_and_triaged_goes_back_to_needs_triage_keeping_its_other_labels() { + for triaged in [ + &["ready-for-human", "architecture"][..], + &["Architecture", "WONTFIX", "Architect-Idea"], + &["needs-info"], + &[], + ] { + let scenario = scenario(); + scenario.issue_labelled(7, triaged); + let url = scenario.issue_url(7); + scenario.agent_does(&ends_with(&format!( + "Architecture review already filed: {url}" + ))); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!( + result.stdout, + format!("{url}\n"), + "stderr: {}", + result.stderr + ); + let kept = triaged + .iter() + .filter(|label| !label.eq_ignore_ascii_case(ARCHITECT_IDEA)) + .copied(); + let expected: Vec<&str> = kept.chain(["needs-triage", ARCHITECT_IDEA]).collect(); + assert_eq!(scenario.issue_labels(7), expected, "{triaged:?}"); + } +} + +#[test] +fn the_architect_idea_label_is_created_when_the_repository_lacks_it_and_kept_when_it_has_it() { + for (repo_labels, created) in [ + (&["needs-triage"][..], true), + (&["needs-triage", "Architect-Idea"], false), + ] { + let scenario = scenario(); + scenario.repo_has_labels(repo_labels); + let url = scenario.issue_url(7); + scenario.agent_does(&ends_with(&format!("Architecture review idea: {url}"))); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let creates = scenario.gh_calls_of("label", "create"); + if created { + assert_eq!(scenario.repo_labels(), ["needs-triage", ARCHITECT_IDEA]); + assert_eq!(creates.len(), 1, "{creates:?}"); + assert_eq!(creates[0][2], ARCHITECT_IDEA); + } else { + assert_eq!(scenario.repo_labels(), repo_labels); + assert!(creates.is_empty(), "{creates:?}"); + } + assert_eq!(scenario.issue_labels(7), ["needs-triage", ARCHITECT_IDEA]); + } +} + +#[test] +fn an_idea_that_cannot_be_labelled_fails_the_architect_run_naming_the_cause() { + for (failing, cause) in [ + ("label list", "gh label list failed: HTTP 502"), + ( + "api --method", + "gh api --method PUT repos/acme/widgets/issues/7/labels --silent \ + -f labels[]=needs-triage -f labels[]=architect-idea failed: HTTP 502", + ), + ] { + let scenario = scenario(); + let url = scenario.issue_url(7); + scenario.agent_does(&ends_with(&format!( + "Architecture review already filed: {url}" + ))); + scenario.gh_fails(failing); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, ""); + let cause = format!("thirdshift: could not label the Architect idea #7: {cause}"); + assert!( + result.stderr.contains(&cause), + "expected {cause:?} in stderr: {}", + result.stderr + ); + assert!(scenario.issue_labels(7).is_empty(), "{failing}"); + assert_nothing_left_behind(&scenario); + } +} + #[test] fn a_review_with_no_strong_candidate_dispatches_nothing_without_plan_only() { let scenario = scenario(); scenario.issue_labelled(7, &["needs-triage"]); let url = scenario.issue_url(7); scenario.agent_does(&ends_with(&format!("Architecture review idea: {url}"))); - let github = scenario.gh_state(); let result = scenario.run(&["architect", "merge"]); @@ -582,7 +683,8 @@ fn a_review_with_no_strong_candidate_dispatches_nothing_without_plan_only() { &url, &format!("no Strong candidate: the Architecture review filed the idea {url}"), ); - assert_eq!(scenario.gh_state(), github); + assert_eq!(scenario.issue_labels(7), ["needs-triage", ARCHITECT_IDEA]); + assert!(scenario.gh_calls_of("pr", "create").is_empty()); } #[test] @@ -1825,7 +1927,7 @@ fn an_architect_run_on_a_different_repository_at_the_same_time_is_not_skipped() let gadgets = scenario.path("gh-state-gadgets.json"); fs::write( &gadgets, - r#"{"repo": "acme/gadgets", "issues": {}, "prs": []}"#, + r#"{"repo": "acme/gadgets", "issues": {"3": "OPEN"}, "prs": []}"#, ) .unwrap(); @@ -2039,9 +2141,9 @@ fn an_architect_plan_that_is_closed_or_no_longer_labelled_lets_the_architect_run } #[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. +fn a_plan_left_needing_triage_does_not_skip_the_architect_run() { + // #7 is the plan a review that failed left half-published: it was never + // labelled an Architect plan, nor an Architect idea. let scenario = scenario(); scenario.issue_labelled(7, &["needs-triage", "architecture"]); scenario.agent_does(AGENT_PUBLISHES_A_TICKET); @@ -2120,3 +2222,203 @@ fn an_architect_run_skipped_for_open_plans_sends_one_notification_naming_each_an "{text}" ); } + +/// Make issue `number` an open Architect idea titled `title`, waiting for +/// triage, as thirdshift left the idea of an earlier Architect run. +fn waiting_architect_idea(scenario: &Scenario, number: u32, title: &str) { + scenario.issue_is(number, "OPEN"); + scenario.issue_titled(number, title); + scenario.issue_labelled(number, &["architecture", "needs-triage", ARCHITECT_IDEA]); +} + +/// What a skipped Architect run says of the Architect idea `number`, titled +/// `title`, waiting for triage on the scenario's repository. +fn waiting_for_triage(scenario: &Scenario, number: u32, title: &str) -> String { + format!( + "Architect idea #{number} \"{title}\" is waiting for triage: {}", + scenario.issue_url(number) + ) +} + +#[test] +fn an_architect_idea_waiting_for_triage_skips_the_architect_run_before_any_review() { + for args in [ + vec!["architect"], + vec!["architect", "--plan-only"], + vec!["architect", "the Spec run", "merge", "base", "main"], + ] { + let scenario = scenario(); + waiting_architect_idea(&scenario, 7, "Deepen the session module"); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + 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!( + after_start(&result.stderr), + format!( + "thirdshift: {}\n", + waiting_for_triage(&scenario, 7, "Deepen the session module") + ) + ); + assert!(scenario.claude_calls().is_empty(), "claude was started"); + assert_eq!(scenario.gh_state(), github); + assert_nothing_left_behind(&scenario); + } +} + +#[test] +fn a_skipped_architect_run_names_each_architect_idea_waiting_for_triage_and_prints_its_url() { + let scenario = scenario(); + waiting_architect_idea(&scenario, 5, "Deepen the worktree module"); + waiting_architect_idea(&scenario, 7, "Deepen the session module"); + // Triaged: not waiting. + scenario.issue_is(6, "OPEN"); + scenario.issue_labelled(6, &["ready-for-human", ARCHITECT_IDEA]); + + 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!( + after_start(&result.stderr), + format!( + "thirdshift: {}; {}\n", + waiting_for_triage(&scenario, 7, "Deepen the session module"), + waiting_for_triage(&scenario, 5, "Deepen the worktree module") + ) + ); + assert!(scenario.claude_calls().is_empty(), "claude was started"); +} + +#[test] +fn an_architect_idea_labelled_in_another_case_skips_the_architect_run_too() { + let scenario = scenario(); + scenario.issue_is(7, "OPEN"); + scenario.issue_titled(7, "Deepen the session module"); + scenario.issue_labelled(7, &["Needs-Triage", "Architect-Idea"]); + 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!("{}\n", scenario.issue_url(7))); + assert!(scenario.claude_calls().is_empty(), "claude was started"); +} + +#[test] +fn an_architect_idea_triaged_whatever_the_decision_or_closed_lets_the_architect_run_go_ahead() { + for triage in [ + (|scenario| scenario.issue_labelled(7, &["ready-for-agent", ARCHITECT_IDEA])) + as fn(&Scenario), + |scenario| scenario.issue_labelled(7, &["ready-for-human", ARCHITECT_IDEA]), + |scenario| scenario.issue_labelled(7, &["Needs-Info", "Architect-Idea"]), + |scenario| scenario.issue_labelled(7, &["wontfix", ARCHITECT_IDEA]), + |scenario| scenario.issue_labelled(7, &["architecture", ARCHITECT_IDEA]), + |scenario| scenario.issue_is(7, "CLOSED"), + ] { + let scenario = scenario(); + waiting_architect_idea(&scenario, 7, "Deepen the session module"); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + triage(&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 with_an_open_architect_plan_and_an_architect_idea_waiting_the_skip_reports_the_plan() { + let scenario = scenario(); + open_architect_plan(&scenario, 5, "Deepen the worktree module"); + waiting_architect_idea(&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", scenario.issue_url(5))); + assert_eq!( + after_start(&result.stderr), + format!( + "thirdshift: {}\n", + still_open(&scenario, 5, "Deepen the worktree module") + ) + ); +} + +#[test] +fn the_next_architect_run_after_a_review_that_filed_an_idea_is_skipped_until_the_idea_is_triaged() { + let scenario = scenario(); + scenario.agent_does_in_session( + 1, + r#" +url=$(gh issue create --title "Deepen the session module" --body "The idea" --label needs-triage) +printf 'Architecture review idea: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + scenario.agent_does_in_session(2, AGENT_PUBLISHES_A_TICKET); + let first = scenario.run(&["architect", "--plan-only"]); + assert_eq!( + first.stdout, + format!("{PLAN_URL}\n"), + "stderr: {}", + first.stderr + ); + + let second = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(second.code, Some(0), "stderr: {}", second.stderr); + assert_eq!(second.stdout, format!("{PLAN_URL}\n")); + assert!( + second.stderr.contains("is waiting for triage"), + "stderr: {}", + second.stderr + ); + assert_eq!(scenario.claude_calls().len(), 1, "a session was started"); + + scenario.issue_labelled(8, &["ready-for-human", ARCHITECT_IDEA]); + let third = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(third.code, Some(0), "stderr: {}", third.stderr); + assert_eq!(third.stdout, format!("{}\n", scenario.issue_url(9))); + assert_eq!(scenario.claude_calls().len(), 2); +} + +#[test] +fn an_idea_that_cannot_be_labelled_sends_one_notification_with_the_cause() { + let scenario = scenario(); + let url = scenario.issue_url(7); + scenario.agent_does(&ends_with(&format!( + "Architecture review already filed: {url}" + ))); + scenario.gh_fails("api --method"); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: review failed" + ); + assert!( + text.starts_with( + "Review: failed\n\ + Cause: could not label the Architect idea #7: gh api --method PUT" + ), + "{text}" + ); +} diff --git a/tests/command_surface.rs b/tests/command_surface.rs index c277c12d9..499d8b636 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -93,7 +93,10 @@ fn help_documents_architect_its_focus_the_dispatch_plan_only_and_when_an_archite "With --plan-only, the Architect run prints the plan's URL and stops instead", "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", + "thirdshift prints that issue's URL instead", + "Either way thirdshift labels that issue an Architect idea: architect-idea and needs-triage", + "an Architect run that finds an Architect idea open and still labelled needs-triage is skipped as well", + "Any triage decision lets the next Architect run go ahead", "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",