diff --git a/README.md b/README.md index d337d9b06..ad57a57d2 100644 --- a/README.md +++ b/README.md @@ -129,7 +129,8 @@ It sends one [Run notification](#run-notifications), whatever the outcome. To ha The other commands: ```sh -thirdshift architect [] --plan-only # review the Base branch's architecture and publish a plan (see Architect runs) +thirdshift architect [] # review the Base branch's architecture, publish a plan and run it (see Architect runs) +thirdshift architect [] --plan-only # publish the plan, mark it ready and stop there thirdshift email-test [
] # send a test email through Resend (see Email) thirdshift setup # choose your defaults and write the User config with every setting (see User config) thirdshift update # update to the latest release (see Updating) @@ -137,7 +138,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, with the URL of the issue it ended on, its plan or its idea, on stdout in place of a pull request's. +`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. 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. @@ -346,15 +347,21 @@ Running the Spec again picks up where the last Spec run stopped. The Spec branch ## Architect runs -`thirdshift architect --plan-only` starts an **Architect run**: the factory looks for architecture work itself, with no **Issue URL**. Start it from a clone of the repository, on the **Base branch**: +`thirdshift architect` starts an **Architect run**: the factory looks for architecture work itself, with no **Issue URL**, publishes a plan for the top opportunity, and implements it. Start it from a clone of the repository, on the **Base branch**: ```sh cd ~/repos/widgets -thirdshift architect --plan-only # review the whole codebase -thirdshift architect "the Spec run" --plan-only # point the review at an area +thirdshift architect # review the whole codebase, then run the plan +thirdshift architect "the Spec run" # point the review at an area +thirdshift architect merge parallel 2 # merge the plan's pull request, two Tickets at once +thirdshift architect "the Spec run" --plan-only # publish the plan, mark it ready and stop ``` -The focus is optional free text, given as one argument, before or after `--plan-only`; it goes into the Session prompt. `--plan-only` is required for now: an Architect run publishes its plan and stops, and can't yet go on to implement it. `architect` is a command only as the first argument. A second focus, `--plan-only` twice, any other argument starting with a dash, or an empty focus is an argument error (exit `2`), as is leaving `--plan-only` out. +The focus is optional free text, given as one argument, anywhere among the flags; it goes into the Session prompt. `architect` is a command only as the first argument. + +`merge`, `no-merge` and `parallel ` (or `--merge`, `--no-merge` and `--parallel `) are for the run the plan is dispatched as, and mean what they do for `thirdshift `, so none of them is ever read as the focus. `--plan-only` stops the Architect run once the plan is marked ready, and dispatches nothing. + +A second focus, a repeated flag, `merge` with `no-merge`, `parallel` without a whole number from 1 up, any other argument starting with a dash, or an empty focus is an argument error (exit `2`). So is `merge`, `no-merge` or `parallel` with `--plan-only`, since nothing is dispatched for them to apply to. An Architect run: @@ -364,14 +371,19 @@ An Architect run: 4. 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`). 5. 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. Removes the worktree and the temporary plugin directory, whatever the outcome. +7. Dispatches the plan, unless given `--plan-only`, exactly as `thirdshift ` would from the same clone: a [Spec run](#spec-runs) when the plan has sub-issues, a Run otherwise. `merge` and `no-merge` apply to the Spec PR or the Run's pull request, and `parallel ` to the Spec run, as if given to that command, and the [User config](#user-config) sets what they leave unsaid: `merge.always`, `spec.parallel`, `launch.pull` and `logs.dir`. The one difference is that no [Run notification](#run-notifications) is sent, whatever `email.always` says. + +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. + +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 `. -Once the plan is marked ready it 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 `. Progress lines on stderr say when the review starts, which plan it reported, and when the labels are swapped; with no Strong candidate, the last line says which issue the Architect run ended on instead. +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. No label is changed, so a plan the review did publish stays `needs-triage` for you to finish or close. +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. ### 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: exit `0`, with one issue's URL alone on stdout. thirdshift changes no label on that issue, and 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 changes no label on that issue 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: `. stdout carries the idea issue's URL, and stderr's last line is `no Strong candidate: the Architecture review filed the idea `. - **Already filed.** An open issue already covers that recommendation, so the review files nothing and ends its final message with `Architecture review already filed: `. stdout carries that issue's URL, and stderr's last line is `no Strong candidate: already covers the Architecture review's top recommendation, so it filed nothing`. diff --git a/src/architect.rs b/src/architect.rs index b43db5056..97518f821 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -1,9 +1,10 @@ -//! One Architect run: an Architecture review of the Base branch, 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. 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. +//! An Architect run up to its plan: an Architecture review of the Base +//! branch, 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 +//! named instead: the idea issue it filed for its top recommendation, or the +//! open issue that already covers it. use std::fmt; use std::path::{Path, PathBuf}; diff --git a/src/args.rs b/src/args.rs index 2eddee5d3..857cbea6d 100644 --- a/src/args.rs +++ b/src/args.rs @@ -1,5 +1,5 @@ //! The command line: which command, and for a Run, its Issue URL and flags, -//! or for an Architect run, its focus. +//! or for an Architect run, its focus and flags. use std::mem::discriminant; use std::num::NonZeroUsize; @@ -32,6 +32,17 @@ pub struct ArchitectArgs { /// The free text that points the Architecture review at an area, if /// given. pub focus: Option, + /// The flags for the Spec run or Run the plan is dispatched as, or none + /// with [`PLAN_ONLY`], which dispatches nothing. + pub dispatch: Option, +} + +/// What an Architect run's flags ask of the Spec run or Run it dispatches +/// its plan as, each meaning what it does in [`RunArgs`]. +#[derive(Debug, Default, PartialEq, Eq)] +pub struct DispatchArgs { + pub goal: Option, + pub parallel: Option, } /// The hidden argument a Spec run starts each Ticket's Run with, followed by @@ -103,17 +114,7 @@ pub fn parse(args: &[String]) -> Result { "no-email" | "--no-email" => { ask_once(&mut email, NotificationAsk::Skip, arg, EMAIL_FLAGS)? } - "parallel" | "--parallel" => { - if parallel.is_some() { - bail!("repeated argument: {arg}"); - } - let n = args.next(); - let Some(n) = n.and_then(|n| n.parse::().ok()) else { - let given = n.map(|n| format!(", not {n}")).unwrap_or_default(); - bail!("{arg} must be followed by a whole number from 1 up{given}"); - }; - parallel = Some(n); - } + "parallel" | "--parallel" => ask_parallel(&mut parallel, arg, args.next())?, SPEC_BRANCH => { if spec_branch.is_some() { bail!("repeated argument: {arg}"); @@ -140,34 +141,51 @@ pub fn parse(args: &[String]) -> Result { })) } -/// Parse the arguments after `architect`: at most one focus, and -/// [`PLAN_ONLY`] once, in either order. Nothing dispatches a plan yet, so -/// without the flag there is nothing an Architect run could go on to do, and -/// it is asked for. Any other argument that starts with a dash is unexpected -/// rather than a focus. +/// Parse the arguments after `architect`: at most one focus, and its flags, +/// each at most once, in any order. [`PLAN_ONLY`] dispatches nothing, so the +/// flags for the dispatched run, `merge`, `no-merge` and `parallel`, which a +/// Run takes too, can't go with it. They are never the focus, and any other +/// argument that starts with a dash is unexpected rather than a focus. fn parse_architect(args: &[String]) -> Result { let mut focus = None; let mut plan_only = false; - for arg in args { - if arg == PLAN_ONLY { - if plan_only { - bail!("repeated argument: {arg}"); + let mut dispatch = DispatchArgs::default(); + let mut args = args.iter(); + while let Some(arg) = args.next() { + match arg.as_str() { + PLAN_ONLY => { + if plan_only { + bail!("repeated argument: {arg}"); + } + plan_only = true; + } + "merge" | "--merge" => ask_once(&mut dispatch.goal, Goal::Merged, arg, MERGE_FLAGS)?, + "no-merge" | "--no-merge" => { + ask_once(&mut dispatch.goal, Goal::ReadyForReview, arg, MERGE_FLAGS)? } - plan_only = true; - } else if arg.starts_with('-') { - bail!("unexpected argument after architect: {arg}"); - } else if focus.is_some() { - bail!("unexpected argument after the focus: {arg}"); - } else if arg.trim().is_empty() { - bail!("the focus is empty"); - } else { - focus = Some(arg.clone()); + "parallel" | "--parallel" => ask_parallel(&mut dispatch.parallel, arg, args.next())?, + _ if arg.starts_with('-') => bail!("unexpected argument after architect: {arg}"), + _ if focus.is_some() => bail!("unexpected argument after the focus: {arg}"), + _ if arg.trim().is_empty() => bail!("the focus is empty"), + _ => focus = Some(arg.clone()), } } if !plan_only { - bail!("architect needs {PLAN_ONLY}: it can't yet implement the plan it publishes"); + return Ok(ArchitectArgs { + focus, + dispatch: Some(dispatch), + }); + } + if dispatch != DispatchArgs::default() { + bail!( + "merge, no-merge and parallel can't be used with {PLAN_ONLY}: \ + it dispatches no run for them to apply to" + ); } - Ok(ArchitectArgs { focus }) + Ok(ArchitectArgs { + focus, + dispatch: None, + }) } const MERGE_FLAGS: &str = "merge and no-merge"; @@ -187,6 +205,20 @@ fn ask_once(given: &mut Option, asked: T, arg: &str, flags: &str) -> Resul Ok(()) } +/// Record in `parallel` the number `n` that follows the flag `arg`: a whole +/// number from 1 up, given once. +fn ask_parallel(parallel: &mut Option, arg: &str, n: Option<&String>) -> Result<()> { + if parallel.is_some() { + bail!("repeated argument: {arg}"); + } + let Some(n) = n.and_then(|n| n.parse::().ok()) else { + let given = n.map(|n| format!(", not {n}")).unwrap_or_default(); + bail!("{arg} must be followed by a whole number from 1 up{given}"); + }; + *parallel = Some(n); + Ok(()) +} + /// Is `arg`, after `email`, the address to send to? Only if it looks like /// one, so the Issue URL is never taken for it. fn is_address(arg: &str) -> bool { @@ -238,16 +270,110 @@ mod tests { (vec!["architect", "the Spec run", "--plan-only"], focus()), (vec!["architect", "--plan-only", "the Spec run"], focus()), ] { - assert_eq!(architect_args(&args).focus, expected, "{args:?}"); + let architect_args = architect_args(&args); + assert_eq!(architect_args.focus, expected, "{args:?}"); + assert_eq!(architect_args.dispatch, None, "{args:?}"); + } + } + + #[test] + fn architect_without_plan_only_dispatches_its_plan_with_or_without_a_focus() { + for (args, focus) in [ + (vec!["architect"], None), + ( + vec!["architect", "the Spec run"], + Some("the Spec run".to_string()), + ), + ] { + let architect_args = architect_args(&args); + assert_eq!(architect_args.focus, focus, "{args:?}"); + assert_eq!( + architect_args.dispatch, + Some(DispatchArgs { + goal: None, + parallel: None + }), + "{args:?}" + ); + } + } + + #[test] + fn architect_takes_the_merge_and_parallel_flags_for_the_run_it_dispatches() { + let two = NonZeroUsize::new(2); + for (args, goal, parallel) in [ + (vec!["architect", "merge"], Some(Goal::Merged), None), + (vec!["architect", "--merge"], Some(Goal::Merged), None), + ( + vec!["architect", "no-merge"], + Some(Goal::ReadyForReview), + None, + ), + ( + vec!["architect", "--no-merge"], + Some(Goal::ReadyForReview), + None, + ), + (vec!["architect", "parallel", "2"], None, two), + ( + vec!["architect", "--parallel", "2", "the Spec run", "merge"], + Some(Goal::Merged), + two, + ), + ] { + assert_eq!( + architect_args(&args).dispatch, + Some(DispatchArgs { goal, parallel }), + "{args:?}" + ); + } + let args = ["architect", "--parallel", "2", "the Spec run", "merge"]; + assert_eq!(architect_args(&args).focus.as_deref(), Some("the Spec run")); + } + + #[test] + fn architect_rejects_contradictory_repeated_and_malformed_dispatch_flags() { + for (args, error) in [ + ( + vec!["architect", "merge", "--no-merge"], + "merge and no-merge can't be used together", + ), + ( + vec!["architect", "no-merge", "the Spec run", "merge"], + "merge and no-merge can't be used together", + ), + ( + vec!["architect", "merge", "--merge"], + "repeated argument: --merge", + ), + ( + vec!["architect", "parallel", "2", "parallel", "3"], + "repeated argument: parallel", + ), + ( + vec!["architect", "parallel"], + "parallel must be followed by a whole number from 1 up", + ), + ( + vec!["architect", "parallel", "the Spec run"], + "parallel must be followed by a whole number from 1 up, not the Spec run", + ), + ] { + assert_eq!(rejection(&args), error, "{args:?}"); } } #[test] - fn architect_without_plan_only_is_rejected_as_nothing_implements_the_plan_yet() { - for args in [vec!["architect"], vec!["architect", "the Spec run"]] { + fn architect_rejects_the_dispatch_flags_with_plan_only_which_dispatches_nothing() { + for args in [ + vec!["architect", "--plan-only", "merge"], + vec!["architect", "--no-merge", "--plan-only"], + vec!["architect", "the Spec run", "parallel", "2", "--plan-only"], + ] { assert_eq!( rejection(&args), - "architect needs --plan-only: it can't yet implement the plan it publishes", + "merge, no-merge and parallel can't be used with --plan-only: \ + it dispatches no run for them to apply to", "{args:?}" ); } @@ -265,8 +391,8 @@ mod tests { "repeated argument: --plan-only", ), ( - vec!["architect", "--merge", "--plan-only"], - "unexpected argument after architect: --merge", + vec!["architect", "--email", "--plan-only"], + "unexpected argument after architect: --email", ), (vec!["architect", " ", "--plan-only"], "the focus is empty"), ] { diff --git a/src/main.rs b/src/main.rs index d0315433f..72571f3bb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -33,7 +33,7 @@ use args::{ArchitectArgs, Command, RunArgs}; use config::UserConfig; use failed_run::FailedRun; use notification::{NotificationAsk, RunNotification}; -use run::Goal; +use run::{Goal, Reached}; use spec_run::Parallel; const HELP: &str = "\ @@ -43,7 +43,8 @@ usage: thirdshift Run the factory on the iss thirdshift merge Run the factory on the issue, then merge its pull request thirdshift --no-merge Run the factory on the issue and leave its pull request for review thirdshift --email Run the factory on the issue, then email how the Run ended - thirdshift architect [] --plan-only Review the Base branch's architecture and publish a plan for a refactor + thirdshift architect [] Review the Base branch's architecture, publish a plan for a refactor, and run it + thirdshift architect [] --plan-only Publish and mark ready the plan for a refactor, and stop there thirdshift email-test [
] Send a test email through Resend, to check the email setup thirdshift setup Choose your defaults, then write the User config with every setting thirdshift update Update thirdshift to the latest release @@ -84,11 +85,16 @@ 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 prints its URL, to read, edit and run with thirdshift . --plan-only is -required: an Architect run can't yet implement its plan. is free text, one argument, +and dispatches it as thirdshift 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. +merge, --no-merge and parallel apply to that run, as do the User config's defaults, +except that it sends no Run notification, whatever email.always says; +parallel fails it if the plan is a single Ticket. With --plan-only, the Architect run +prints the plan's URL and stops instead, for you to read, edit and run with +thirdshift , and takes none of those flags. is free text, one argument, that points the review at an area: - thirdshift architect \"the Spec run\" --plan-only + thirdshift architect \"the Spec run\" 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 @@ -187,41 +193,30 @@ fn main() -> ExitCode { Ok(notification) => notification, Err(error) => return failure(&error), }; - let parallel = Parallel { - tickets: parallel.unwrap_or(config.spec_parallel), - asked: parallel.is_some(), - }; let ended = run::run( &issue, goal, &config.logs_dir, launch_pull, - parallel, + Parallel::new(parallel, config.spec_parallel), spec_branch.as_deref(), ); - let code = match &ended { - Ok(reached) => { - // Also on stderr, so the outcome shows even when stdout is captured. - progress::step(format_args!( - "PR {} is {}", - reached.pr_url, - reached.goal.outcome() - )); - print_url(&reached.pr_url); - ExitCode::SUCCESS - } - Err(failed) => report(failed), - }; + let code = run_outcome(&ended); if let Some(notification) = notification { notification.send(&ended); } code } -/// An Architect run that stops at the plan: the URL of the issue it ended on -/// on stdout, the plan once it is marked ready or, with no Strong candidate, -/// the idea issue the review filed or the issue that already covers its top -/// recommendation, or the cause and the session log on stderr. +/// An Architect run: the Architecture review and its plan marked ready, then, +/// unless the command asked to stop at the plan, the plan dispatched as +/// `thirdshift ` with the same flags would be, whose ending is the +/// Architect run's, though it sends no Run notification. One that stops at +/// the plan, or whose review found no Strong candidate and so published no +/// plan 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. fn architect(args: &ArchitectArgs) -> ExitCode { let config = match user_config() { Ok(config) => config, @@ -230,14 +225,46 @@ fn architect(args: &ArchitectArgs) -> ExitCode { if let Err(error) = interrupt::install() { return failure(&error); } - match architect::run(args.focus.as_deref(), &config.logs_dir, config.launch_pull) { - Ok(outcome) => { + let outcome = match architect::run(args.focus.as_deref(), &config.logs_dir, config.launch_pull) + { + Ok(outcome) => outcome, + Err(failed) => return report(&failed), + }; + let (architect::Outcome::PlanReady(plan), Some(dispatch)) = (&outcome, &args.dispatch) else { + // Also on stderr, so the outcome shows even when stdout is captured. + progress::step(format_args!("{outcome}")); + print_url(outcome.url()); + return ExitCode::SUCCESS; + }; + progress::step(format_args!( + "dispatching the plan {url}, as thirdshift {url} would", + url = plan.url + )); + run_outcome(&run::run( + plan, + dispatch.goal.unwrap_or(config.default_goal()), + &config.logs_dir, + config.launch_pull, + Parallel::new(dispatch.parallel, config.spec_parallel), + None, + )) +} + +/// How a Run or a Spec run that `ended` shows: its pull request's URL on +/// stdout once it reached its goal, or as a Failed run does. +fn run_outcome(ended: &Result) -> ExitCode { + match ended { + Ok(reached) => { // Also on stderr, so the outcome shows even when stdout is captured. - progress::step(format_args!("{outcome}")); - print_url(outcome.url()); + progress::step(format_args!( + "PR {} is {}", + reached.pr_url, + reached.goal.outcome() + )); + print_url(&reached.pr_url); ExitCode::SUCCESS } - Err(failed) => report(&failed), + Err(failed) => report(failed), } } diff --git a/src/spec_run.rs b/src/spec_run.rs index 59576c359..5b7d637b0 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -54,6 +54,17 @@ pub struct Parallel { pub asked: bool, } +impl Parallel { + /// As many Tickets at once as the command `asked` for with + /// `parallel `, else as the User config's `default` says. + pub fn new(asked: Option, default: NonZeroUsize) -> Self { + Parallel { + tickets: asked.unwrap_or(default), + asked: asked.is_some(), + } + } +} + /// Whether there are `tickets`, as a Spec has, and every one is closed. pub fn all_closed(tickets: &[Ticket]) -> bool { !tickets.is_empty() && tickets.iter().all(|ticket| !ticket.is_open) diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 0e340fbe7..b2cd431ec 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -1,9 +1,11 @@ -//! An Architect run that stops at the plan: `thirdshift architect --plan-only` -//! runs the Architecture review in its own worktree, detached at the Base -//! branch's head on origin, then checks the plan the review published, swaps -//! its `needs-triage` for `ready-for-agent`, and prints its URL. A review -//! with no Strong candidate has no plan: the Architect run prints the URL of -//! the idea issue it filed, or of the open issue that already covers it. +//! 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. +//! Without, it dispatches the plan as `thirdshift ` 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, +//! or of the open issue that already covers it, and dispatches nothing. mod support; @@ -14,6 +16,9 @@ use support::{REPO, RunResult, Scenario}; /// The first issue the fake agent creates: the scenario starts with issue #7. 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"; + const NO_FINAL_LINE: &str = "the Architecture review ended without the final line its prompt asks for"; @@ -30,6 +35,93 @@ fn publishes_a_ticket_then(then: &str) -> String { format!("{AGENT_PUBLISHES_A_TICKET}{then}\n") } +/// A script in which the agent for issue `issue` commits its work and opens +/// its PR into `base`, leaving the pushing to thirdshift. +fn agent_opens_pr(issue: u32, base: &str) -> String { + format!( + r#" +echo "{issue}" > issue-{issue}.txt +git add issue-{issue}.txt +git commit -q -m "Work on {issue}" +gh pr create --base {base} --head issue-{issue} --title "Work on {issue}" --body "Closes #{issue}" +"# + ) +} + +/// The Architecture review, the first session, publishes a single Ticket as +/// the plan, #8, and the session that implements #8 opens its PR into `main`. +fn single_ticket_plan() -> Scenario { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, AGENT_PUBLISHES_A_TICKET); + scenario.agent_does_for(8, &agent_opens_pr(8, "main")); + scenario +} + +/// The Architecture review, the first session, publishes a Spec, #8, with +/// two Tickets that don't block each other, #9 and #10, as the plan. Each +/// Ticket's session opens its PR into the Spec branch, #10's after doing +/// `before_ticket_10`. +fn spec_plan(before_ticket_10: &str) -> Scenario { + let scenario = Scenario::new(); + scenario.agent_does_in_session( + 1, + r#" +spec=$(gh issue create --title "Deepen the session module" --body "The Spec" --label needs-triage) +gh issue create --title "Move the logs" --body "A Ticket" --label ready-for-agent +gh issue create --title "Move the sessions" --body "A Ticket" --label ready-for-agent +gh fake sub-issues 8 '[9, 10]' +printf 'Architecture review plan: %s\n' "$spec" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + scenario.agent_does_for(9, &agent_opens_pr(9, "issue-8")); + scenario.agent_does_for( + 10, + &format!("{before_ticket_10}\n{}", agent_opens_pr(10, "issue-8")), + ); + scenario +} + +/// The newest pull request from `head` on the fake GitHub. +fn pr_from(scenario: &Scenario, head: &str) -> serde_json::Value { + let prs = scenario.gh_state()["prs"].clone(); + let from_head = prs + .as_array() + .unwrap() + .iter() + .rfind(|pr| pr["head"] == head); + from_head + .unwrap_or_else(|| panic!("no PR from {head}: {prs}")) + .clone() +} + +/// The issue number each agent session after the Architecture review was +/// for, the first Issue URL its prompt names, in order. +fn dispatched_sessions(scenario: &Scenario) -> Vec { + scenario.claude_calls()[1..] + .iter() + .map(|call| { + let prompt = call["prompt"].as_str().unwrap(); + let (_, after) = prompt.split_once("/issues/").unwrap(); + after.chars().take_while(char::is_ascii_digit).collect() + }) + .collect() +} + +/// Assert the Architect run ended with `pr` in `outcome`, as a Run or a Spec +/// run that reached its goal does: exit 0, the PR's URL alone on stdout, and +/// the outcome as the last line on stderr. +fn assert_ended_with_pr(result: &RunResult, pr: &serde_json::Value, outcome: &str) { + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let url = pr["url"].as_str().unwrap(); + assert_eq!(result.stdout, format!("{url}\n")); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: PR {url} is {outcome}").as_str()), + "stderr: {}", + result.stderr + ); +} + /// A script in which the agent ends with `final_message`, publishing nothing. fn ends_with(final_message: &str) -> String { format!("printf '%s\\n' '{final_message}' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n") @@ -350,6 +442,25 @@ fn a_review_whose_idea_is_already_filed_prints_that_issues_url_and_files_and_cha assert_eq!(scenario.gh_state(), github); } +#[test] +fn a_review_with_no_strong_candidate_dispatches_nothing_without_plan_only() { + let scenario = Scenario::new(); + 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"]); + + assert_ended_without_a_plan( + &scenario, + &result, + &url, + &format!("no Strong candidate: the Architecture review filed the idea {url}"), + ); + assert_eq!(scenario.gh_state(), github); +} + #[test] fn a_closed_plan_is_refused() { let scenario = Scenario::new(); @@ -502,3 +613,184 @@ fn launch_pull_brings_the_launch_directorys_base_branch_up_to_date_first() { result.stderr ); } + +#[test] +fn a_single_ticket_plan_starts_a_run_on_it_and_ends_as_that_run_does() { + let scenario = single_ticket_plan(); + + let result = scenario.run(&["architect"]); + + 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"]); + let calls = scenario.claude_calls(); + assert_eq!(calls.len(), 2, "sessions: {calls:?}"); + let prompt = calls[1]["prompt"].as_str().unwrap(); + assert!( + prompt.starts_with(&format!("/thirdshift:implement {PLAN_URL}\n")), + "{prompt}" + ); + assert_eq!(calls[1]["branch"], "issue-8"); + assert_eq!(pr["base"], "main"); + assert_eq!(pr["state"], "OPEN"); + assert_eq!( + scenario.origin_log("issue-8").unwrap()[0], + "Work on 8".to_string() + ); + scenario.assert_cleaned_up("issue-8"); +} + +#[test] +fn a_plan_with_tickets_starts_a_spec_run_on_it_and_ends_as_that_spec_run_does() { + let scenario = spec_plan(""); + + let result = scenario.run(&["architect"]); + + let spec_pr = pr_from(&scenario, "issue-8"); + assert_ended_with_pr(&result, &spec_pr, "ready for review"); + assert_eq!(spec_pr["base"], "main"); + assert_eq!(spec_pr["state"], "OPEN"); + assert_eq!(spec_pr["isDraft"], false); + assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + 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"); + assert!( + dispatched.is_some() && dispatched < started, + "stderr: {}", + result.stderr + ); + let mut sessions = dispatched_sessions(&scenario); + assert_eq!(sessions.pop().as_deref(), Some("8"), "the Spec review"); + sessions.sort(); + assert_eq!(sessions, ["10", "9"]); + let gh = scenario.gh_state(); + for ticket in [9, 10] { + let pr = pr_from(&scenario, &format!("issue-{ticket}")); + assert_eq!(pr["base"], "issue-8"); + assert_eq!(pr["state"], "MERGED"); + assert_eq!(gh["issues"][ticket.to_string()], "CLOSED"); + assert_eq!( + scenario.origin_file("issue-8", &format!("issue-{ticket}.txt")), + Some(format!("{ticket}\n")) + ); + } + scenario.assert_cleaned_up("issue-8"); +} + +#[test] +fn merge_merges_the_runs_pull_request_or_the_spec_pr() { + for scenario in [single_ticket_plan(), spec_plan("")] { + let result = scenario.run(&["architect", "merge"]); + + let pr = pr_from(&scenario, "issue-8"); + assert_ended_with_pr(&result, &pr, "merged"); + assert_eq!(pr["base"], "main"); + assert_eq!(pr["state"], "MERGED"); + assert!( + scenario.origin_log("issue-8").is_none(), + "issue-8 is still on origin" + ); + } +} + +#[test] +fn the_user_configs_merge_default_applies_unless_no_merge_is_given() { + for plan in [single_ticket_plan, || spec_plan("")] { + for (args, outcome, state) in [ + (vec!["architect"], "merged", "MERGED"), + (vec!["architect", "--no-merge"], "ready for review", "OPEN"), + ] { + let scenario = plan(); + scenario.user_config_is("[merge]\nalways = true\n"); + + let result = scenario.run(&args); + + let pr = pr_from(&scenario, "issue-8"); + assert_ended_with_pr(&result, &pr, outcome); + assert_eq!(pr["state"], state, "{args:?}"); + } + } +} + +#[test] +fn parallel_passes_through_to_the_spec_run() { + // #10's session only succeeds if #9 landed on the Spec branch before it + // started, as it has only when the Tickets run one at a time. + let scenario = spec_plan("test -f issue-9.txt"); + + let result = scenario.run(&["architect", "parallel", "1"]); + + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-8"), "ready for review"); + assert_eq!(dispatched_sessions(&scenario), ["9", "10", "8"]); +} + +#[test] +fn parallel_on_a_single_ticket_plan_fails_as_it_does_for_an_issue_that_is_not_a_spec() { + let scenario = single_ticket_plan(); + + let result = scenario.run(&["architect", "parallel", "2"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, ""); + assert_eq!( + result.stderr.lines().last(), + Some("thirdshift: parallel is only for a Spec, and #8 has no sub-issues"), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.claude_calls().len(), 1, "a Run was started"); + assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + assert!(scenario.origin_log("issue-8").is_none()); + assert_nothing_left_behind(&scenario); +} + +#[test] +fn a_dispatched_run_that_fails_fails_the_architect_run_as_a_failed_run_does() { + let scenario = single_ticket_plan(); + scenario.agent_does_for(8, &format!("{}exit 3\n", agent_opens_pr(8, "main"))); + + let result = scenario.run(&["architect"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert!( + result.stderr.contains("thirdshift: claude exited 3\n"), + "stderr: {}", + result.stderr + ); + let last = result.stderr.lines().last().unwrap(); + assert!( + last.starts_with("thirdshift: session log: ") && last.ends_with("-implement.jsonl"), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); +} + +#[test] +fn progress_lines_show_the_dispatch_after_the_label_swap_and_before_the_run() { + let scenario = single_ticket_plan(); + + let result = scenario.run(&["architect"]); + + 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" + .to_string(), + format!("thirdshift: dispatching the plan {PLAN_URL}, as thirdshift {PLAN_URL} would\n"), + "thirdshift: implement: session started\n".to_string(), + ] { + let Some(at) = rest.find(&line) else { + panic!("expected {line:?}, in order, in stderr: {}", result.stderr); + }; + rest = &rest[at + line.len()..]; + } + assert!( + !result.stderr.contains("is ready for an agent"), + "stderr: {}", + result.stderr + ); +} diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 92b0bbf02..293307cb0 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -12,6 +12,7 @@ fn assert_help_text(text: &str) { "thirdshift ", "thirdshift merge ", "thirdshift --email ", + "thirdshift architect []", "thirdshift architect [] --plan-only", "thirdshift email-test [
]", "thirdshift setup", @@ -68,7 +69,7 @@ fn help_does_not_mention_the_flag_aliases() { } #[test] -fn help_documents_architect_its_focus_plan_only_and_one_architect_run_at_a_time() { +fn help_documents_architect_its_focus_the_dispatch_plan_only_and_one_architect_run_at_a_time() { let scenario = Scenario::new(); let help = scenario.run(&["help"]).stdout; @@ -77,8 +78,10 @@ fn help_documents_architect_its_focus_plan_only_and_one_architect_run_at_a_time( let help = help.split_whitespace().collect::>().join(" "); for mention in [ " is free text", - "--plan-only is required", - "thirdshift architect \"the Spec run\" --plan-only", + "dispatches it as thirdshift would: a Spec run on a Spec, a Run on a single Ticket", + "merge, --no-merge and parallel apply to that run", + "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", "Start one Architect run per repository at a time", @@ -87,22 +90,41 @@ fn help_documents_architect_its_focus_plan_only_and_one_architect_run_at_a_time( } } +const PLAN_ONLY_DISPATCHES_NOTHING: &str = "merge, no-merge and parallel can't be used with --plan-only: \ + it dispatches no run for them to apply to"; + #[test] fn architect_with_arguments_it_cant_use_prints_an_error_and_the_help_to_stderr() { let scenario = Scenario::new(); for (args, error) in [ - ( - vec!["architect"], - "architect needs --plan-only: it can't yet implement the plan it publishes", - ), ( vec!["architect", "the Spec run", "the Run", "--plan-only"], "unexpected argument after the focus: the Run", ), ( - vec!["architect", "--plan-only", "--merge"], - "unexpected argument after architect: --merge", + vec!["architect", "--plan-only", "--email"], + "unexpected argument after architect: --email", + ), + ( + vec!["architect", "merge", "no-merge"], + "merge and no-merge can't be used together", + ), + ( + vec!["architect", "parallel", "0"], + "parallel must be followed by a whole number from 1 up, not 0", + ), + ( + vec!["architect", "--plan-only", "merge"], + PLAN_ONLY_DISPATCHES_NOTHING, + ), + ( + vec!["architect", "no-merge", "--plan-only"], + PLAN_ONLY_DISPATCHES_NOTHING, + ), + ( + vec!["architect", "--plan-only", "parallel", "2"], + PLAN_ONLY_DISPATCHES_NOTHING, ), ] { let result = scenario.run(&args); diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 17296f5b0..1fd773731 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -121,6 +121,8 @@ //! address or null //! gh fake labels '' set issue 's labels //! gh fake created