From fecb5d28527c7ddb5925df4783b0cae23e2c4c1f Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sun, 4 Oct 2026 21:18:42 -0400 Subject: [PATCH 1/2] The Architect run's conclusion views and labels through the Pass seam, tested in-process (#386) Co-Authored-By: Claude Opus 5.5 --- src/architect.rs | 375 +++++++++++++++++++++++++++++++++++++---- src/github.rs | 5 +- src/labels.rs | 6 - src/pass.rs | 74 ++++++-- tests/architect_run.rs | 105 ------------ 5 files changed, 406 insertions(+), 159 deletions(-) diff --git a/src/architect.rs b/src/architect.rs index 3e6460a8d..6e89b754c 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -23,10 +23,10 @@ use anyhow::{Context, Result, anyhow, bail}; use chrono::{DateTime, Utc}; use crate::failed_run::FailedRun; -use crate::github::{self, ListedIssue}; +use crate::github::ListedIssue; use crate::interrupt; use crate::issue::{IssueUrl, Repo}; -use crate::labels::{Edit, Label, NEEDS_TRIAGE, READY_FOR_AGENT}; +use crate::labels::{Edit, Label, Labels, NEEDS_TRIAGE, READY_FOR_AGENT}; use crate::launch::{self, AlreadyRunning, Launch, Start}; use crate::logs::{self, Pass, Work}; use crate::pass::{OnGitHub, Outside}; @@ -247,7 +247,15 @@ pub fn run( } let worktree = ReviewWorktree::create(directory.git(), &repo.name, &base)?; let logs = Logs::of_architect_run(&repo); - let (reviewed, log) = review(worktree, &base, focus, directory.origin(), started, &logs); + let (reviewed, log) = review( + &mut on_github, + worktree, + &base, + focus, + directory.origin(), + started, + &logs, + ); reviewed.map(Outcome::Reviewed).map_err(|error| FailedRun { log, ..FailedRun::from(error) @@ -293,10 +301,11 @@ fn skip(repo: &Repo, skipped: Skipped) -> Outcome { } /// The Architecture review session in `worktree`, of the Base branch `base`, -/// then [`conclude`] on its final message. The worktree is removed once the +/// then [`conclude`] on its final message, through `outside`. The worktree is removed once the /// review is concluded. Returns how it ended with the most recent session's /// log, if a session created it. fn review( + outside: &mut impl Outside, worktree: ReviewWorktree, base: &str, focus: Option<&str>, @@ -313,7 +322,7 @@ fn review( } let prompt = prompt::architecture_review(base, focus); let final_message = sessions.run_to_final_message(REVIEW, &prompt)?; - conclude(final_message.as_deref(), origin, started, base) + conclude(outside, final_message.as_deref(), origin, started, base) }) } @@ -349,12 +358,14 @@ 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 -/// 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. +/// last line of its `final_message` names, reaching it through `outside`: a +/// plan is marked ready, and an idea issue, or the issue that already covers +/// the top recommendation, is labelled an Architect idea. Fails, with no +/// call made, if the session had no final message or its last line is not +/// one the prompt asks for, and fails if the plan can't be marked ready or +/// the idea labelled. fn conclude( + outside: &mut impl Outside, final_message: Option<&str>, origin: &str, started: DateTime, @@ -362,16 +373,16 @@ fn conclude( ) -> Result { match final_message.and_then(Report::read) { Some(Report::Plan(plan)) => { - mark_plan_ready(&plan, origin, started)?; + mark_plan_ready(outside, &plan, origin, started)?; let base = base.to_string(); Ok(Reviewed::PlanReady { plan, base }) } Some(Report::Idea(idea)) => { - label_idea(&idea)?; + label_idea(outside, &idea)?; Ok(Reviewed::IdeaFiled(idea)) } Some(Report::AlreadyFiled(issue)) => { - label_idea(&issue)?; + label_idea(outside, &issue)?; Ok(Reviewed::AlreadyFiled(issue)) } None => bail!("the Architecture review ended without the final line its prompt asks for"), @@ -380,53 +391,78 @@ fn conclude( /// Mark `plan` ready: check it, then swap its `needs-triage` for /// `ready-for-agent` and label it `architect-plan`, in one request that -/// keeps its other labels, as read once the checks are done, having added -/// each label it puts on to the repository if it lacks it. Fails, changing -/// no label, if the plan fails its checks. -fn mark_plan_ready(plan: &IssueUrl, origin: &str, started: DateTime) -> Result<()> { - progress::step(format_args!( +/// keeps its other labels, as viewed for the checks, having added each label +/// it puts on to the repository if it lacks it. Fails, changing no label, if +/// the plan fails its checks. +fn mark_plan_ready( + outside: &mut impl Outside, + plan: &IssueUrl, + origin: &str, + started: DateTime, +) -> Result<()> { + outside.step(format!( "the Architecture review published the plan {}", plan.url )); - check_plan(plan, origin, started)?; - if interrupt::requested() { + let labels = check_plan(outside, plan, origin, started)?; + if outside.interrupted() { bail!("interrupted"); } - progress::step(format_args!( + outside.step(format!( "marking the plan ready: swapping {NEEDS_TRIAGE} for {READY_FOR_AGENT} and adding {ARCHITECT_PLAN} on #{}", plan.number )); - Edit::read(plan, &[NEEDS_TRIAGE], &[READY_FOR_AGENT, ARCHITECT_PLAN])?.apply() + outside.apply(&Edit::of( + plan, + labels, + &[NEEDS_TRIAGE], + &[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 each 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() { +/// on it, in one request that keeps its other labels, as viewed, having +/// added each 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(outside: &mut impl Outside, idea: &IssueUrl) -> Result<()> { + if outside.interrupted() { bail!("interrupted"); } - progress::step(format_args!( + outside.step(format!( "labelling #{} an Architect idea: adding {NEEDS_TRIAGE} and {ARCHITECT_IDEA}", idea.number )); - Edit::read(idea, &[], &[NEEDS_TRIAGE, ARCHITECT_IDEA]) - .and_then(|edit| edit.apply()) + outside + .issue(idea) + .and_then(|viewed| { + outside.apply(&Edit::of( + idea, + viewed.labels, + &[], + &[NEEDS_TRIAGE, ARCHITECT_IDEA], + )) + }) .with_context(|| format!("could not label the Architect idea #{}", idea.number)) } -/// Check `plan`: fails unless it is in the repository at `origin`, open, -/// created since the Architect run `started`, and has no label but -/// `needs-triage` that makes an Unready Ticket. -fn check_plan(plan: &IssueUrl, origin: &str, started: DateTime) -> Result<()> { +/// Check `plan`, viewing it: fails unless it is in the repository at +/// `origin`, which is checked before the view, open, created since the +/// Architect run `started`, and has no label but `needs-triage` that makes +/// an Unready Ticket. Returns its labels, as viewed. +fn check_plan( + outside: &mut impl Outside, + plan: &IssueUrl, + origin: &str, + started: DateTime, +) -> Result { if !plan.matches_origin(origin) { bail!( "the plan {} is not in the repository at origin {origin}", plan.url ); } - let issue = github::issue(plan)?; + let issue = outside.issue(plan)?; if !issue.is_open { bail!("the plan {} is closed", plan.url); } @@ -440,12 +476,13 @@ fn check_plan(plan: &IssueUrl, origin: &str, started: DateTime) -> Result<( if let Some(unready) = issue.labels.swapped(&[NEEDS_TRIAGE], &[]).unready() { bail!("the plan {} is labelled {unready}", plan.url); } - Ok(()) + Ok(issue.labels) } #[cfg(test)] mod tests { use super::*; + use crate::github; use crate::pass::{Call, InMemory}; const URL: &str = "https://github.com/acme/widgets/issues/8"; @@ -634,4 +671,270 @@ mod tests { assert!(line.unwrap().starts_with("Architect idea #6")); assert_eq!(urls, [url(6)]); } + + const ORIGIN: &str = "git@github.com:acme/widgets.git"; + + /// When the Architect run started. + fn started() -> DateTime { + DateTime::parse_from_rfc3339("2026-10-04T09:00:00.750Z") + .unwrap() + .to_utc() + } + + /// An issue as viewed: open or not, created `after` seconds after the + /// Architect run started, labelled `labels`. + fn viewed(open: bool, after: i64, labels: &[&str]) -> github::Issue { + github::Issue { + is_open: open, + labels: labels.iter().copied().collect(), + created: started() + chrono::Duration::seconds(after), + } + } + + /// The final message of a review whose last line is `line` and the URL + /// of issue 8. + fn message(line: &str) -> String { + format!("Reviewed the codebase.\n\n{line}{}\n", url(8)) + } + + /// The Architect run's conclusion on `final_message`, through `repo`: + /// how the review ended, or the cause it failed with, and what it did. + fn concluded(mut repo: InMemory, final_message: &str) -> (Result, Vec) { + let ended = conclude(&mut repo, Some(final_message), ORIGIN, started(), "main") + .map(|reviewed| reviewed.to_string()) + .map_err(|error| format!("{error:#}")); + (ended, repo.calls) + } + + fn step(line: &str) -> Call { + Call::Step(line.to_string()) + } + + fn edit(issue: u64, off: &[&'static str], on: &[&'static str], labels: &[&str]) -> Call { + Call::Edit { + issue, + off: off.to_vec(), + on: on.to_vec(), + labels: labels.iter().map(|label| label.to_string()).collect(), + } + } + + const PUBLISHED: &str = + "the Architecture review published the plan https://github.com/acme/widgets/issues/8"; + const MARKING: &str = "marking the plan ready: swapping needs-triage for ready-for-agent and adding architect-plan on #8"; + const LABELLING: &str = + "labelling #8 an Architect idea: adding needs-triage and architect-idea"; + + #[test] + fn a_plan_is_viewed_once_and_marked_ready_in_one_edit_keeping_its_other_labels() { + let repo = InMemory::default().viewable( + 8, + viewed(true, 0, &["architecture", "needs-triage", "Spec"]), + ); + + let (ended, calls) = concluded(repo, &message(prompt::PLAN_LINE)); + + assert_eq!( + ended.unwrap(), + format!("plan {} is ready for an agent", url(8)) + ); + assert_eq!( + calls, + [ + step(PUBLISHED), + Call::View(8), + step(MARKING), + edit( + 8, + &["needs-triage"], + &["ready-for-agent", "architect-plan"], + &["architecture", "Spec", "ready-for-agent", "architect-plan"], + ), + ] + ); + } + + #[test] + fn a_plan_with_needs_triage_in_another_case_has_it_taken_off() { + let repo = InMemory::default().viewable(8, viewed(true, 0, &["Needs-Triage"])); + + let (ended, calls) = concluded(repo, &message(prompt::PLAN_LINE)); + + assert!(ended.is_ok()); + assert_eq!( + calls.last().unwrap(), + &edit( + 8, + &["needs-triage"], + &["ready-for-agent", "architect-plan"], + &["ready-for-agent", "architect-plan"], + ) + ); + } + + #[test] + fn a_plan_in_another_repository_is_refused_without_a_view() { + let repo = InMemory::default().viewable(8, viewed(true, 0, &["needs-triage"])); + let other = "Architecture review plan: https://github.com/other/widgets/issues/8"; + + let (ended, calls) = concluded(repo, other); + + assert_eq!( + ended.unwrap_err(), + format!( + "the plan https://github.com/other/widgets/issues/8 is not in the repository at origin {ORIGIN}" + ) + ); + assert_eq!( + calls, + [step( + "the Architecture review published the plan https://github.com/other/widgets/issues/8" + )] + ); + } + + #[test] + fn a_plan_failing_a_check_is_refused_with_its_cause_and_no_edit() { + for (plan, cause) in [ + (viewed(false, 0, &["needs-triage"]), "is closed"), + ( + viewed(true, -1, &["needs-triage"]), + "was created before this Architect run started", + ), + ( + viewed(true, 0, &["needs-triage", "ready-for-human"]), + "is labelled ready-for-human", + ), + ( + viewed(true, 0, &["needs-triage", "Needs-Info"]), + "is labelled needs-info", + ), + (viewed(true, 0, &["wontfix"]), "is labelled wontfix"), + ] { + let repo = InMemory::default().viewable(8, plan); + + let (ended, calls) = concluded(repo, &message(prompt::PLAN_LINE)); + + assert_eq!(ended.unwrap_err(), format!("the plan {} {cause}", url(8))); + assert_eq!(calls, [step(PUBLISHED), Call::View(8)], "{cause}"); + } + } + + #[test] + fn a_plan_created_in_the_second_the_run_started_passes() { + // The run started 0.75 s into its second; GitHub gives the plan's + // creation to the second. + let mut plan = viewed(true, 0, &["needs-triage"]); + plan.created = DateTime::parse_from_rfc3339("2026-10-04T09:00:00Z") + .unwrap() + .to_utc(); + let repo = InMemory::default().viewable(8, plan); + + let (ended, _) = concluded(repo, &message(prompt::PLAN_LINE)); + + assert!(ended.is_ok(), "{ended:?}"); + } + + #[test] + fn an_interrupt_after_the_plans_checks_makes_no_edit() { + let repo = InMemory::default() + .viewable(8, viewed(true, 0, &["needs-triage"])) + .interrupted(); + + let (ended, calls) = concluded(repo, &message(prompt::PLAN_LINE)); + + assert_eq!(ended.unwrap_err(), "interrupted"); + assert_eq!(calls, [step(PUBLISHED), Call::View(8)]); + } + + #[test] + fn an_idea_filed_or_already_filed_is_labelled_an_architect_idea_keeping_its_other_labels() { + for (line, ending, labels, after) in [ + ( + prompt::IDEA_LINE, + format!( + "no Strong candidate: the Architecture review filed the idea {}", + url(8) + ), + vec!["architecture", "needs-triage"], + vec!["architecture", "needs-triage", "architect-idea"], + ), + ( + prompt::ALREADY_FILED_LINE, + format!( + "no Strong candidate: {} already covers the Architecture review's top recommendation, so it filed nothing", + url(8) + ), + vec!["bug", "ready-for-agent"], + vec!["bug", "ready-for-agent", "needs-triage", "architect-idea"], + ), + ] { + let repo = InMemory::default().viewable(8, viewed(true, -3600, &labels)); + + let (ended, calls) = concluded(repo, &message(line)); + + let on: Vec<&'static str> = ["needs-triage", "architect-idea"] + .into_iter() + .filter(|label| !labels.contains(label)) + .collect(); + assert_eq!(ended.unwrap(), ending); + assert_eq!( + calls, + [step(LABELLING), Call::View(8), edit(8, &[], &on, &after),], + "{line}" + ); + } + } + + #[test] + fn an_idea_that_cannot_be_viewed_or_edited_fails_naming_it_ahead_of_the_cause() { + let unviewable = InMemory::default(); + let (ended, calls) = concluded(unviewable, &message(prompt::IDEA_LINE)); + assert_eq!( + ended.unwrap_err(), + "could not label the Architect idea #8: gh: Could not resolve to an issue with the number of 8" + ); + assert_eq!(calls, [step(LABELLING), Call::View(8)]); + + let uneditable = InMemory::default() + .viewable(8, viewed(true, 0, &[])) + .edit_failing(8); + let (ended, calls) = concluded(uneditable, &message(prompt::ALREADY_FILED_LINE)); + assert_eq!( + ended.unwrap_err(), + "could not label the Architect idea #8: gh: could not edit #8" + ); + assert_eq!(calls.len(), 3); + assert!(matches!(calls[2], Call::Edit { issue: 8, .. })); + } + + #[test] + fn an_interrupt_before_an_idea_is_viewed_makes_no_call() { + let repo = InMemory::default() + .viewable(8, viewed(true, 0, &[])) + .interrupted(); + + let (ended, calls) = concluded(repo, &message(prompt::IDEA_LINE)); + + assert_eq!(ended.unwrap_err(), "interrupted"); + assert_eq!(calls, []); + } + + #[test] + fn a_final_message_without_the_line_as_asked_for_fails_with_no_call_made() { + let repo = InMemory::default().viewable(8, viewed(true, 0, &["needs-triage"])); + + let (ended, calls) = concluded(repo, "Published the plan."); + + assert_eq!( + ended.unwrap_err(), + "the Architecture review ended without the final line its prompt asks for" + ); + assert_eq!(calls, []); + + let mut repo = InMemory::default(); + let ended = conclude(&mut repo, None, ORIGIN, started(), "main"); + assert!(ended.is_err()); + assert_eq!(repo.calls, []); + } } diff --git a/src/github.rs b/src/github.rs index 8e0ec35e5..344d252bb 100644 --- a/src/github.rs +++ b/src/github.rs @@ -43,8 +43,9 @@ pub fn issue_title(issue: &IssueUrl) -> Result { .to_string()) } -/// An issue as an Architect run reads the plan its Architecture review -/// published, and as a Claim reads its issue after a Self-merge. +/// An issue as an Architect run reads the issue its Architecture review +/// ended on, and as a Claim reads its issue after a Self-merge. +#[derive(Clone)] pub struct Issue { pub is_open: bool, pub labels: Labels, diff --git a/src/labels.rs b/src/labels.rs index 8340d1d4b..effbaa166 100644 --- a/src/labels.rs +++ b/src/labels.rs @@ -145,12 +145,6 @@ enum Request { } impl<'a> Edit<'a> { - /// Taking each of `off` off `issue` and putting each of `on` on it, - /// against its labels as read now. - pub fn read(issue: &'a IssueUrl, off: &[Label], on: &[Label]) -> Result> { - Ok(Edit::of(issue, github::issue_labels(issue)?, off, on)) - } - /// Taking each of `off` off `issue` and putting each of `on` on it, /// against `labels`, which the caller has just read. pub fn of(issue: &'a IssueUrl, labels: Labels, off: &[Label], on: &[Label]) -> Edit<'a> { diff --git a/src/pass.rs b/src/pass.rs index d29cd13b5..68fd16787 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -1,26 +1,35 @@ //! The Pass seam: what an Architect run's or a Pickup run's gates, which -//! decide before any work whether it is skipped, read and change of GitHub. -//! [`OnGitHub`] does each through `gh`, the Ready issue search and the -//! progress lines; [`InMemory`], in tests, from memory, recording each call. +//! decide before any work whether it is skipped, read and change of GitHub, +//! and what an Architect run's conclusion after its Architecture review +//! does, viewing and labelling the issue the review ended on. +//! [`OnGitHub`] does each through `gh`, the Ready issue search, the +//! interrupt flag and the progress lines; [`InMemory`], in tests, from +//! memory, recording each call. use anyhow::Result; use crate::git::Git; -use crate::github::{self, ListedIssue}; -use crate::issue::Repo; +use crate::github::{self, Issue, ListedIssue}; +use crate::interrupt; +use crate::issue::{IssueUrl, Repo}; use crate::labels::{Edit, Label}; use crate::progress; use crate::ready::{self, ReadyIssue}; -/// What a pass's gates read and change of its repository on GitHub, and -/// where their progress lines go. +/// What a pass's gates, and an Architect run's conclusion, read and change of +/// its repository on GitHub, whether the run is interrupted, and where their +/// progress lines go. pub trait Outside { /// Every open issue in the repository labelled `label`. fn open_issues(&mut self, label: Label) -> Result>; /// Every closed issue in the repository labelled `label`. fn closed_issues(&mut self, label: Label) -> Result>; + /// `issue`'s state, labels and when it was created. + fn issue(&mut self, issue: &IssueUrl) -> Result; /// Make `edit`. fn apply(&mut self, edit: &Edit) -> Result<()>; + /// Whether the run was interrupted. + fn interrupted(&mut self) -> bool; /// The repository's lowest-numbered Ready issue, if it has one, by the /// Ready issue search, with its lines on the issues passed over. fn ready_issue(&mut self) -> Result>; @@ -43,10 +52,18 @@ impl Outside for OnGitHub<'_> { github::closed_issues_labelled(&self.repo.slug(), label) } + fn issue(&mut self, issue: &IssueUrl) -> Result { + github::issue(issue) + } + fn apply(&mut self, edit: &Edit) -> Result<()> { edit.apply() } + fn interrupted(&mut self) -> bool { + interrupt::requested() + } + fn ready_issue(&mut self) -> Result> { ready::first(self.launch, self.repo) } @@ -64,7 +81,7 @@ mod in_memory { use anyhow::{Result, bail}; use super::Outside; - use crate::github::ListedIssue; + use crate::github::{Issue, ListedIssue}; use crate::issue::IssueUrl; use crate::labels::{Edit, Label, Labels}; use crate::ready::ReadyIssue; @@ -76,6 +93,8 @@ mod in_memory { Open(&'static str), /// They listed the closed issues with this label. Closed(&'static str), + /// They viewed this issue. + View(u64), /// They applied an Edit to this issue, taking `off` off it and /// putting `on` on it, which leaves it with `labels`. Edit { @@ -91,12 +110,16 @@ mod in_memory { } /// A repository in memory: its issues, filed by label, open or - /// closed, the Ready issue search's answer, and the issues whose Edits - /// fail. + /// closed, the issues it can view, the Ready issue search's answer, the + /// issues whose Edits fail, and whether the run was interrupted. #[derive(Default)] pub struct InMemory { /// Each issue, filed under the label a listing finds it by. filed: Vec, + /// Each issue it can view, by number. Viewing any other fails. + viewable: Vec<(u64, Issue)>, + /// Whether the run was interrupted. + interrupted: bool, /// Whether listing the closed issues fails. closed_listing_fails: bool, /// The number of each issue an Edit to fails. @@ -138,6 +161,18 @@ mod in_memory { self } + /// Let issue `number` be viewed, as `issue`. + pub fn viewable(mut self, number: u64, issue: Issue) -> Self { + self.viewable.push((number, issue)); + self + } + + /// Have the run interrupted. + pub fn interrupted(mut self) -> Self { + self.interrupted = true; + self + } + /// Make listing the closed issues fail. pub fn closed_listing_failing(mut self) -> Self { self.closed_listing_fails = true; @@ -180,6 +215,21 @@ mod in_memory { Ok(self.labelled(label, false)) } + fn issue(&mut self, issue: &IssueUrl) -> Result { + self.calls.push(Call::View(issue.number)); + match self + .viewable + .iter() + .find(|(number, _)| *number == issue.number) + { + Some((_, viewed)) => Ok(viewed.clone()), + None => bail!( + "gh: Could not resolve to an issue with the number of {}", + issue.number + ), + } + } + fn apply(&mut self, edit: &Edit) -> Result<()> { let (off, on) = edit.changes(); self.calls.push(Call::Edit { @@ -194,6 +244,10 @@ mod in_memory { Ok(()) } + fn interrupted(&mut self) -> bool { + self.interrupted + } + fn ready_issue(&mut self) -> Result> { self.calls.push(Call::ReadySearch); Ok(self diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 0a4f9f521..e4670fc70 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -243,28 +243,6 @@ fn plan_only_marks_the_published_ticket_ready_and_prints_its_url() { assert_nothing_left_behind(&scenario); } -#[test] -fn plan_only_marks_a_published_spec_ready_and_leaves_its_tickets_and_other_labels() { - let scenario = scenario(); - scenario.agent_does( - r#" -spec=$(gh issue create --title "Deepen the session module" --body "The Spec" --label needs-triage,architecture) -gh issue create --title "Move the logs" --body "A Ticket" --label ready-for-agent -printf 'Architecture review plan: %s\n' "$spec" > "$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!( - scenario.issue_labels(8), - ["architecture", "ready-for-agent", ARCHITECT_PLAN] - ); - assert_eq!(scenario.issue_labels(9), ["ready-for-agent"]); -} - #[test] fn the_architect_plan_label_is_created_when_the_repository_lacks_it() { let scenario = scenario(); @@ -617,38 +595,6 @@ fn a_review_whose_idea_is_already_filed_prints_that_issues_url_files_nothing_and 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 [ @@ -753,21 +699,6 @@ fn a_review_with_no_strong_candidate_dispatches_nothing_without_plan_only() { assert!(scenario.gh_calls_of("pr", "create").is_empty()); } -#[test] -fn a_closed_plan_is_refused() { - let scenario = scenario(); - scenario.agent_does(&publishes_a_ticket_then("gh fake issue 8 CLOSED")); - - let result = scenario.run(&["architect", "--plan-only"]); - - assert_failed( - &scenario, - &result, - &format!("the plan {PLAN_URL} is closed"), - ); - assert_eq!(scenario.issue_labels(8), ["needs-triage"]); -} - #[test] fn a_plan_older_than_the_architect_run_is_refused() { let scenario = scenario(); @@ -786,42 +717,6 @@ fn a_plan_older_than_the_architect_run_is_refused() { assert_eq!(scenario.issue_labels(7), ["needs-triage"]); } -#[test] -fn a_plan_with_another_unready_label_is_refused() { - for label in ["ready-for-human", "needs-info", "wontfix"] { - let scenario = scenario(); - scenario.agent_does(&publishes_a_ticket_then(&format!( - r#"gh fake labels 8 '["needs-triage", "{label}"]'"# - ))); - - let result = scenario.run(&["architect", "--plan-only"]); - - assert_failed( - &scenario, - &result, - &format!("the plan {PLAN_URL} is labelled {label}"), - ); - assert_eq!(scenario.issue_labels(8), ["needs-triage", label]); - } -} - -#[test] -fn a_plan_in_another_repository_is_refused() { - let scenario = scenario(); - let url = "https://github.com/acme/gadgets/issues/8"; - scenario.agent_does(&ends_with(&format!("Architecture review plan: {url}"))); - - let result = scenario.run(&["architect", "--plan-only"]); - - assert_failed( - &scenario, - &result, - &format!( - "the plan {url} is not in the repository at origin https://github.com/acme/widgets.git" - ), - ); -} - #[test] fn a_detached_head_is_rejected_before_any_work() { let scenario = scenario(); From a47237865cac4f5b950255593fe94eea300f7117 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sun, 4 Oct 2026 21:27:45 -0400 Subject: [PATCH 2/2] Review clean-ups for the Architect run's conclusion (#386) Co-Authored-By: Claude Opus 5.5 --- src/architect.rs | 114 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 91 insertions(+), 23 deletions(-) diff --git a/src/architect.rs b/src/architect.rs index 6e89b754c..b679b3db1 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -301,9 +301,9 @@ fn skip(repo: &Repo, skipped: Skipped) -> Outcome { } /// The Architecture review session in `worktree`, of the Base branch `base`, -/// then [`conclude`] on its final message, through `outside`. The worktree is removed once the -/// review is concluded. Returns how it ended with the most recent session's -/// log, if a session created it. +/// then [`conclude`] on its final message, through `outside`. The worktree is +/// removed once the review is concluded. Returns how it ended with the most +/// recent session's log, if a session created it. fn review( outside: &mut impl Outside, worktree: ReviewWorktree, @@ -849,15 +849,13 @@ mod tests { #[test] fn an_idea_filed_or_already_filed_is_labelled_an_architect_idea_keeping_its_other_labels() { - for (line, ending, labels, after) in [ + for (line, ending) in [ ( prompt::IDEA_LINE, format!( "no Strong candidate: the Architecture review filed the idea {}", url(8) ), - vec!["architecture", "needs-triage"], - vec!["architecture", "needs-triage", "architect-idea"], ), ( prompt::ALREADY_FILED_LINE, @@ -865,47 +863,109 @@ mod tests { "no Strong candidate: {} already covers the Architecture review's top recommendation, so it filed nothing", url(8) ), - vec!["bug", "ready-for-agent"], - vec!["bug", "ready-for-agent", "needs-triage", "architect-idea"], ), ] { - let repo = InMemory::default().viewable(8, viewed(true, -3600, &labels)); + let repo = InMemory::default() + .viewable(8, viewed(true, -3600, &["architecture", "needs-triage"])); let (ended, calls) = concluded(repo, &message(line)); - let on: Vec<&'static str> = ["needs-triage", "architect-idea"] - .into_iter() - .filter(|label| !labels.contains(label)) - .collect(); assert_eq!(ended.unwrap(), ending); assert_eq!( calls, - [step(LABELLING), Call::View(8), edit(8, &[], &on, &after),], + [ + step(LABELLING), + Call::View(8), + edit( + 8, + &[], + &["architect-idea"], + &["architecture", "needs-triage", "architect-idea"], + ), + ], "{line}" ); } } #[test] - fn an_idea_that_cannot_be_viewed_or_edited_fails_naming_it_ahead_of_the_cause() { - let unviewable = InMemory::default(); - let (ended, calls) = concluded(unviewable, &message(prompt::IDEA_LINE)); + fn an_idea_already_filed_and_triaged_goes_back_to_needs_triage_keeping_its_other_labels() { + for (triaged, on, after) in [ + ( + &["ready-for-human", "architecture"][..], + &["needs-triage", "architect-idea"][..], + &[ + "ready-for-human", + "architecture", + "needs-triage", + "architect-idea", + ][..], + ), + ( + &["Architecture", "WONTFIX", "Architect-Idea"], + &["needs-triage"], + &["Architecture", "WONTFIX", "needs-triage", "architect-idea"], + ), + ( + &["needs-info"], + &["needs-triage", "architect-idea"], + &["needs-info", "needs-triage", "architect-idea"], + ), + ( + &[], + &["needs-triage", "architect-idea"], + &["needs-triage", "architect-idea"], + ), + ] { + let repo = InMemory::default().viewable(8, viewed(true, -3600, triaged)); + + let (ended, calls) = concluded(repo, &message(prompt::ALREADY_FILED_LINE)); + + assert!(ended.is_ok(), "{triaged:?}: {ended:?}"); + assert_eq!( + calls, + [step(LABELLING), Call::View(8), edit(8, &[], on, after)], + "{triaged:?}" + ); + } + } + + #[test] + fn an_idea_that_cannot_be_viewed_fails_naming_it_ahead_of_the_cause() { + let (ended, calls) = concluded(InMemory::default(), &message(prompt::IDEA_LINE)); + assert_eq!( ended.unwrap_err(), "could not label the Architect idea #8: gh: Could not resolve to an issue with the number of 8" ); assert_eq!(calls, [step(LABELLING), Call::View(8)]); + } - let uneditable = InMemory::default() - .viewable(8, viewed(true, 0, &[])) + #[test] + fn an_idea_that_cannot_be_edited_fails_naming_it_ahead_of_the_cause() { + let repo = InMemory::default() + .viewable(8, viewed(true, 0, &["bug"])) .edit_failing(8); - let (ended, calls) = concluded(uneditable, &message(prompt::ALREADY_FILED_LINE)); + + let (ended, calls) = concluded(repo, &message(prompt::ALREADY_FILED_LINE)); + assert_eq!( ended.unwrap_err(), "could not label the Architect idea #8: gh: could not edit #8" ); - assert_eq!(calls.len(), 3); - assert!(matches!(calls[2], Call::Edit { issue: 8, .. })); + assert_eq!( + calls, + [ + step(LABELLING), + Call::View(8), + edit( + 8, + &[], + &["needs-triage", "architect-idea"], + &["bug", "needs-triage", "architect-idea"], + ), + ] + ); } #[test] @@ -931,10 +991,18 @@ mod tests { "the Architecture review ended without the final line its prompt asks for" ); assert_eq!(calls, []); + } + #[test] + fn no_final_message_fails_with_no_call_made() { let mut repo = InMemory::default(); + let ended = conclude(&mut repo, None, ORIGIN, started(), "main"); - assert!(ended.is_err()); + + assert_eq!( + ended.unwrap_err().to_string(), + "the Architecture review ended without the final line its prompt asks for" + ); assert_eq!(repo.calls, []); } }