From 7beb38994087aff0efcc4417d8242e643f6d4978 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 1 Oct 2026 02:48:15 +0000 Subject: [PATCH 1/2] End an Architect run with no Strong candidate on the idea issue or the issue that covers it A review that reports the idea issue it filed, or the open issue that already covers its top idea, no longer fails the Architect run. It swaps no label and dispatches nothing, prints that issue's URL on stdout and exits 0, with a last line on stderr saying which of the two it was. Co-Authored-By: Claude Opus 5.5 --- README.md | 15 ++++-- src/architect.rs | 113 +++++++++++++++++++++++++-------------- src/main.rs | 26 +++++---- tests/architect_run.rs | 78 +++++++++++++++++++-------- tests/command_surface.rs | 2 + 5 files changed, 159 insertions(+), 75 deletions(-) diff --git a/README.md b/README.md index 4142ea20e..903252d52 100644 --- a/README.md +++ b/README.md @@ -137,7 +137,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 plan's URL 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, with the URL of the issue it ended on, its plan or its idea, on stdout in place of a pull request's. 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. @@ -360,14 +360,21 @@ 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. There is no **Origin match**, since there is no Issue URL: the repository is the one `origin` names. With `launch.pull = true` in the [User config](#user-config), it then fast-forwards your checkout of the Base branch, as a Run does. 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 publishes the top recommendation 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. +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 4 and 5 are skipped. 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. -On success 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. +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. -A review session that fails or is interrupted, a final message without that line, 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 that reports an idea it filed, or an open issue that already covers its top recommendation, instead of a plan, is a failure too for now, naming that issue. +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. + +### 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. + +- **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 idea, so it filed nothing`. Start one Architect run per repository at a time. Nothing stops a second one, but two at once may pick the same opportunity and publish the same plan, and the second can't create its worktree while the first's is there. diff --git a/src/architect.rs b/src/architect.rs index 7f4d3729f..8315e8fb9 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -1,7 +1,11 @@ //! 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. +//! 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 it filed, or the open issue that +//! already covers that idea. +use std::fmt; use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; @@ -29,14 +33,53 @@ const REVIEW: &str = "architecture-review"; /// plan passes its checks. const READY_FOR_AGENT: &str = "ready-for-agent"; +/// How an Architect run ended, short of a failure, with the URL of the issue +/// it ended on. Shows as the line that says so. +#[derive(Debug)] +pub enum Outcome { + /// The plan the review published is marked ready. + PlanReady(String), + /// The review found no Strong candidate, and filed its top idea as this + /// issue. + IdeaFiled(String), + /// The review found no Strong candidate, and filed nothing: this open + /// issue already covers its top idea. + AlreadyFiled(String), +} + +impl Outcome { + /// The URL of the issue the Architect run ended on. + pub fn url(&self) -> &str { + match self { + Self::PlanReady(url) | Self::IdeaFiled(url) | Self::AlreadyFiled(url) => url, + } + } +} + +impl fmt::Display for Outcome { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::PlanReady(url) => write!(f, "plan {url} is ready for an agent"), + Self::IdeaFiled(url) => write!( + f, + "no Strong candidate: the Architecture review filed the idea {url}" + ), + Self::AlreadyFiled(url) => write!( + f, + "no Strong candidate: {url} already covers the Architecture review's top idea, so it filed nothing" + ), + } + } +} + /// Run an Architecture review of the branch checked out in the Launch /// directory, the Base branch, pointed at `focus` if given, and mark the plan -/// it publishes ready. Returns the plan's URL. With `launch_pull`, the Launch -/// directory's checkout of the Base branch 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. -pub fn run(focus: Option<&str>, logs_dir: &Path, launch_pull: bool) -> Result { +/// it publishes ready. A review that reports an idea it filed, or the open +/// issue that already covers that idea, instead of a plan, changes no label. With `launch_pull`, the Launch directory's checkout +/// of the Base branch 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. +pub fn run(focus: Option<&str>, logs_dir: &Path, launch_pull: bool) -> Result { let started = Utc::now(); let timestamp = started.format("%Y%m%dT%H%M%SZ").to_string(); let launch = Git::new(std::env::current_dir().context("no current directory")?); @@ -61,7 +104,7 @@ pub fn run(focus: Option<&str>, logs_dir: &Path, launch_pull: bool) -> Result, - origin: &str, - started: DateTime, -) -> Result { - let plan = reported_plan(final_message)?; +/// End the Architect run on the issue the last line of the review's +/// `final_message` names: a plan is marked ready, and an idea, or the issue +/// that already covers it, 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. +fn conclude(final_message: Option<&str>, origin: &str, started: DateTime) -> Result { + match final_message.and_then(Report::read) { + Some(Report::Plan(plan)) => { + mark_plan_ready(&plan, origin, started)?; + Ok(Outcome::PlanReady(plan.url)) + } + Some(Report::Idea(idea)) => Ok(Outcome::IdeaFiled(idea.url)), + Some(Report::AlreadyFiled(issue)) => Ok(Outcome::AlreadyFiled(issue.url)), + None => bail!("the Architecture review ended without the final line its prompt asks for"), + } +} + +/// 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. +fn mark_plan_ready(plan: &IssueUrl, origin: &str, started: DateTime) -> Result<()> { progress::step(format_args!( "the Architecture review published the plan {}", plan.url )); - let kept = labels_to_keep(&plan, origin, started)?; + let kept = labels_to_keep(plan, origin, started)?; if interrupt::requested() { bail!("interrupted"); } @@ -152,26 +206,7 @@ fn mark_plan_ready( .filter(|&label| label != READY_FOR_AGENT) .collect(); labels.push(READY_FOR_AGENT); - github::set_labels(&plan, &labels)?; - Ok(plan.url) -} - -/// The plan the last line of the review's `final_message` names. Fails if -/// the session had no final message, if its last line is not one the prompt -/// asks for, or if that line names an issue that is not a plan. -fn reported_plan(final_message: Option<&str>) -> Result { - match final_message.and_then(Report::read) { - Some(Report::Plan(plan)) => Ok(plan), - Some(Report::Idea(idea)) => bail!( - "the Architecture review published no plan: it filed the idea {}", - idea.url - ), - Some(Report::AlreadyFiled(issue)) => bail!( - "the Architecture review published no plan: {} already covers its top recommendation", - issue.url - ), - None => bail!("the Architecture review ended without a final line naming its plan"), - } + github::set_labels(plan, &labels) } /// The labels `plan` keeps once it is marked ready: all it has but diff --git a/src/main.rs b/src/main.rs index 4012733e6..14c35f780 100644 --- a/src/main.rs +++ b/src/main.rs @@ -90,9 +90,14 @@ that points the review at an area: thirdshift architect \"the Spec run\" --plan-only -A review that fails, is interrupted, or ends without naming a plan fails the Architect run and -leaves any plan it published labelled needs-triage. Start one Architect run per repository at a -time: two at once may publish the same plan. +A review that finds no Strong candidate publishes no plan. It files its top idea as one 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: it filed the idea, or +it filed nothing. + +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. Start one Architect run +per repository at a time: two at once may publish the same plan. The User config, ~/.thirdshift/config.toml, sets defaults for every Run on this machine; thirdshift setup asks for your defaults and writes one listing every setting, to edit. @@ -213,8 +218,10 @@ fn main() -> ExitCode { code } -/// An Architect run that stops at the plan: the plan's URL on stdout once it -/// is marked ready, or the cause and the session log on stderr. +/// 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 the review filed or the issue that already covers it, or the +/// cause and the session log on stderr. fn architect(args: &ArchitectArgs) -> ExitCode { let config = match user_config() { Ok(config) => config, @@ -224,10 +231,10 @@ fn architect(args: &ArchitectArgs) -> ExitCode { return failure(&error); } match architect::run(args.focus.as_deref(), &config.logs_dir, config.launch_pull) { - Ok(plan_url) => { + Ok(outcome) => { // Also on stderr, so the outcome shows even when stdout is captured. - progress::step(format_args!("plan {plan_url} is ready for an agent")); - print_url(&plan_url); + progress::step(format_args!("{outcome}")); + print_url(outcome.url()); ExitCode::SUCCESS } Err(failed) => report(&failed), @@ -262,7 +269,8 @@ fn report(failed: &FailedRun) -> ExitCode { ExitCode::FAILURE } -/// A pull request's or a plan's URL on stdout. A failed write, as once the +/// A pull request's URL, or the URL of the issue an Architect run ended on, +/// on stdout. A failed write, as once the /// terminal has closed, is ignored, so the Run notification still goes. fn print_url(url: &str) { let _ = writeln!(std::io::stdout(), "{url}"); diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 6c2640353..2d79d1a2d 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -1,7 +1,9 @@ //! 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. +//! 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. mod support; @@ -12,7 +14,8 @@ 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"; -const NO_FINAL_LINE: &str = "the Architecture review ended without a final line naming its plan"; +const NO_FINAL_LINE: &str = + "the Architecture review ended without the final line its prompt asks for"; /// The agent publishes a single Ticket as the plan, labelled `needs-triage`, /// and names it in the last line of its final message. @@ -287,30 +290,59 @@ fn a_session_that_ends_without_a_valid_final_line_fails_the_architect_run() { } #[test] -fn a_review_that_published_no_plan_fails_the_architect_run_naming_the_issue_it_reported() { - for (line, cause) in [ - ( - "Architecture review idea: ", - format!("the Architecture review published no plan: it filed the idea {PLAN_URL}"), - ), - ( - "Architecture review already filed: ", +fn a_review_that_files_an_idea_prints_its_url_and_changes_no_label() { + let scenario = Scenario::new(); + scenario.agent_does( + r#" +url=$(gh issue create --title "Deepen the session module" --body "The idea" --label needs-triage) +printf 'No Strong candidate.\n\nArchitecture review idea: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + + 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!( + result.stderr.lines().last(), + Some( format!( - "the Architecture review published no plan: {PLAN_URL} already covers its top recommendation" - ), - ), - ] { - let scenario = Scenario::new(); - scenario.agent_does(&format!( - "gh issue create --title Idea --body Idea --label needs-triage > /dev/null\n{}", - ends_with(&format!("{line}{PLAN_URL}")) - )); + "thirdshift: no Strong candidate: the Architecture review filed the idea {PLAN_URL}" + ) + .as_str() + ) + ); + assert_eq!(scenario.issue_labels(8), ["needs-triage"]); + assert_eq!(scenario.claude_calls().len(), 1); + assert_nothing_left_behind(&scenario); +} - let result = scenario.run(&["architect", "--plan-only"]); +#[test] +fn a_review_whose_idea_is_already_filed_prints_that_issues_url_and_files_and_changes_nothing() { + let scenario = Scenario::new(); + 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(); - assert_failed(&scenario, &result, &cause); - assert_eq!(scenario.issue_labels(8), ["needs-triage"], "{line}"); - } + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{url}\n")); + assert_eq!( + result.stderr.lines().last(), + Some( + format!( + "thirdshift: no Strong candidate: {url} already covers the Architecture review's top idea, so it filed nothing" + ) + .as_str() + ) + ); + assert_eq!(scenario.gh_state(), github); + assert_eq!(scenario.claude_calls().len(), 1); + assert_nothing_left_behind(&scenario); } #[test] diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 9cfef18b7..92b0bbf02 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -79,6 +79,8 @@ fn help_documents_architect_its_focus_plan_only_and_one_architect_run_at_a_time( " is free text", "--plan-only is required", "thirdshift architect \"the Spec run\" --plan-only", + "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", ] { assert!(help.contains(mention), "help lacks {mention:?}: {help}"); From 8287857ef1908b18a8cd06d6b159e5f210da7232 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 1 Oct 2026 03:01:38 +0000 Subject: [PATCH 2/2] Address review: the glossary's top recommendation, the issue kept in the outcome The already-filed line and the docs say top recommendation, as the glossary does. The outcome keeps the Issue URL it was read as, the two no-plan tests share their assertions, and two comments are rewrapped. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++-- src/architect.rs | 50 +++++++++++++++++++++------------------ src/main.rs | 16 ++++++------- tests/architect_run.rs | 53 +++++++++++++++++++++++------------------- 4 files changed, 66 insertions(+), 57 deletions(-) diff --git a/README.md b/README.md index 903252d52..d337d9b06 100644 --- a/README.md +++ b/README.md @@ -365,7 +365,7 @@ An Architect run: 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. -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. +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. 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. @@ -374,7 +374,7 @@ A review session that fails or is interrupted, a final message without one of th 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. - **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 idea, so it filed nothing`. +- **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`. Start one Architect run per repository at a time. Nothing stops a second one, but two at once may pick the same opportunity and publish the same plan, and the second can't create its worktree while the first's is there. diff --git a/src/architect.rs b/src/architect.rs index 8315e8fb9..b43db5056 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -2,8 +2,8 @@ //! 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 it filed, or the open issue that -//! already covers that idea. +//! 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}; @@ -33,40 +33,43 @@ const REVIEW: &str = "architecture-review"; /// plan passes its checks. const READY_FOR_AGENT: &str = "ready-for-agent"; -/// How an Architect run ended, short of a failure, with the URL of the issue -/// it ended on. Shows as the line that says so. +/// How an Architect run ended, short of a failure, with the issue it ended +/// on. Its `Display` is the line that says how it ended, naming that issue. #[derive(Debug)] pub enum Outcome { /// The plan the review published is marked ready. - PlanReady(String), - /// The review found no Strong candidate, and filed its top idea as this - /// issue. - IdeaFiled(String), + PlanReady(IssueUrl), + /// The review found no Strong candidate, and filed its top + /// recommendation as this issue. + IdeaFiled(IssueUrl), /// The review found no Strong candidate, and filed nothing: this open - /// issue already covers its top idea. - AlreadyFiled(String), + /// issue already covers its top recommendation. + AlreadyFiled(IssueUrl), } impl Outcome { /// The URL of the issue the Architect run ended on. pub fn url(&self) -> &str { match self { - Self::PlanReady(url) | Self::IdeaFiled(url) | Self::AlreadyFiled(url) => url, + Self::PlanReady(issue) | Self::IdeaFiled(issue) | Self::AlreadyFiled(issue) => { + &issue.url + } } } } impl fmt::Display for Outcome { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let url = self.url(); match self { - Self::PlanReady(url) => write!(f, "plan {url} is ready for an agent"), - Self::IdeaFiled(url) => write!( + Self::PlanReady(_) => write!(f, "plan {url} is ready for an agent"), + Self::IdeaFiled(_) => write!( f, "no Strong candidate: the Architecture review filed the idea {url}" ), - Self::AlreadyFiled(url) => write!( + Self::AlreadyFiled(_) => write!( f, - "no Strong candidate: {url} already covers the Architecture review's top idea, so it filed nothing" + "no Strong candidate: {url} already covers the Architecture review's top recommendation, so it filed nothing" ), } } @@ -74,9 +77,10 @@ impl fmt::Display for Outcome { /// Run an Architecture review of the branch checked out in the Launch /// directory, the Base branch, pointed at `focus` if given, and mark the plan -/// it publishes ready. A review that reports an idea it filed, or the open -/// issue that already covers that idea, instead of a plan, changes no label. With `launch_pull`, the Launch directory's checkout -/// of the Base branch is first brought up to date with origin. The review's +/// it publishes ready. 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 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. pub fn run(focus: Option<&str>, logs_dir: &Path, launch_pull: bool) -> Result { @@ -168,18 +172,18 @@ impl Report { } /// End the Architect run on the issue the last line of the review's -/// `final_message` names: a plan is marked ready, and an idea, or the issue -/// that already covers it, is left as it is. Fails if the session had no +/// `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. fn conclude(final_message: Option<&str>, origin: &str, started: DateTime) -> Result { match final_message.and_then(Report::read) { Some(Report::Plan(plan)) => { mark_plan_ready(&plan, origin, started)?; - Ok(Outcome::PlanReady(plan.url)) + Ok(Outcome::PlanReady(plan)) } - Some(Report::Idea(idea)) => Ok(Outcome::IdeaFiled(idea.url)), - Some(Report::AlreadyFiled(issue)) => Ok(Outcome::AlreadyFiled(issue.url)), + Some(Report::Idea(idea)) => Ok(Outcome::IdeaFiled(idea)), + Some(Report::AlreadyFiled(issue)) => Ok(Outcome::AlreadyFiled(issue)), None => bail!("the Architecture review ended without the final line its prompt asks for"), } } diff --git a/src/main.rs b/src/main.rs index 14c35f780..d0315433f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -90,10 +90,10 @@ that points the review at an area: thirdshift architect \"the Spec run\" --plan-only -A review that finds no Strong candidate publishes no plan. It files its top idea as one 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: it filed the idea, or -it filed nothing. +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. 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. Start one Architect run @@ -220,8 +220,8 @@ fn main() -> ExitCode { /// 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 the review filed or the issue that already covers it, or the -/// cause and the session log on stderr. +/// the idea issue the review filed or the issue that already covers its top +/// recommendation, or the cause and the session log on stderr. fn architect(args: &ArchitectArgs) -> ExitCode { let config = match user_config() { Ok(config) => config, @@ -270,8 +270,8 @@ fn report(failed: &FailedRun) -> ExitCode { } /// A pull request's URL, or the URL of the issue an Architect run ended on, -/// on stdout. A failed write, as once the -/// terminal has closed, is ignored, so the Run notification still goes. +/// on stdout. A failed write, as once the terminal has closed, is ignored, so +/// the Run notification still goes. fn print_url(url: &str) { let _ = writeln!(std::io::stdout(), "{url}"); } diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 2d79d1a2d..0e340fbe7 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -81,6 +81,22 @@ fn assert_failed(scenario: &Scenario, result: &RunResult, cause: &str) { assert_nothing_left_behind(scenario); } +/// What every Architect run whose review found no Strong candidate shares: +/// exit 0, `url` alone on stdout, `outcome` as stderr's last line, the +/// review as its only session, and nothing left behind. +fn assert_ended_without_a_plan(scenario: &Scenario, result: &RunResult, url: &str, outcome: &str) { + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{url}\n")); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: {outcome}").as_str()), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.claude_calls().len(), 1); + assert_nothing_left_behind(scenario); +} + #[test] fn plan_only_marks_the_published_ticket_ready_and_prints_its_url() { let scenario = Scenario::new(); @@ -292,6 +308,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() { let scenario = Scenario::new(); + let idea = scenario.issue_url(8); scenario.agent_does( r#" url=$(gh issue create --title "Deepen the session module" --body "The idea" --label needs-triage) @@ -301,20 +318,13 @@ printf 'No Strong candidate.\n\nArchitecture review idea: %s\n' "$url" > "$FAKE_ 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!( - result.stderr.lines().last(), - Some( - format!( - "thirdshift: no Strong candidate: the Architecture review filed the idea {PLAN_URL}" - ) - .as_str() - ) + assert_ended_without_a_plan( + &scenario, + &result, + &idea, + &format!("no Strong candidate: the Architecture review filed the idea {idea}"), ); assert_eq!(scenario.issue_labels(8), ["needs-triage"]); - assert_eq!(scenario.claude_calls().len(), 1); - assert_nothing_left_behind(&scenario); } #[test] @@ -329,20 +339,15 @@ fn a_review_whose_idea_is_already_filed_prints_that_issues_url_and_files_and_cha let result = scenario.run(&["architect", "--plan-only"]); - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert_eq!(result.stdout, format!("{url}\n")); - assert_eq!( - result.stderr.lines().last(), - Some( - format!( - "thirdshift: no Strong candidate: {url} already covers the Architecture review's top idea, so it filed nothing" - ) - .as_str() - ) + assert_ended_without_a_plan( + &scenario, + &result, + &url, + &format!( + "no Strong candidate: {url} already covers the Architecture review's top recommendation, so it filed nothing" + ), ); assert_eq!(scenario.gh_state(), github); - assert_eq!(scenario.claude_calls().len(), 1); - assert_nothing_left_behind(&scenario); } #[test]