From 828dde138c12eb85d84a36b2dd021fac6d02b931 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sun, 4 Oct 2026 18:13:46 -0400 Subject: [PATCH 1/2] Test the Architect run's and Pickup run's skip rules in-process behind a Pass seam (#374) A new Pass module holds the one seam both runs' gates read and change GitHub through: the open and closed listings by label, one label Edit, the Ready issue search's answer and a progress line. Each run's gates become one function returning its decision, tested against a shared in-memory adapter that records every call. The Sweep and the Claim-limit count leave the Claim module for the Pickup run's gates. Both entry points record each skip, the lock's included, through one step. The end-to-end tests that only checked gate order or a skip line go, and CONTEXT.md defines the Pass. Co-Authored-By: Claude Opus 5.5 --- CONTEXT.md | 4 + src/architect.rs | 198 +++++++++++++++++++++++++--- src/claim.rs | 41 +----- src/github.rs | 2 +- src/issue.rs | 2 +- src/labels.rs | 25 ++++ src/main.rs | 1 + src/pass.rs | 208 +++++++++++++++++++++++++++++ src/pickup.rs | 289 +++++++++++++++++++++++++++++++++++++++-- tests/architect_run.rs | 167 ------------------------ tests/pickup_run.rs | 101 -------------- 11 files changed, 699 insertions(+), 339 deletions(-) create mode 100644 src/pass.rs diff --git a/CONTEXT.md b/CONTEXT.md index a0df2af13..bdaa7eee0 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -70,6 +70,10 @@ _Avoid_: upkeep, continuous architect, daemon, loop One invocation of the factory with no **Issue URL**: it takes the lowest-numbered **Ready issue** in the repository and starts a **Spec run** or a **Run** on it, as the command on that issue's URL would. One issue per invocation, and thirdshift never schedules itself: the operating system's scheduler starts each one. It is skipped, doing nothing, when another Pickup run or an **Architect run** on the same repository is still running on the machine, its Spec run or Run included, when there is no Ready issue, or when the repository is at its **Claim limit**. Skipped is not a failure, and sends no **Run notification**. _Avoid_: watch, daemon, queue run, poll +**Pass**: +An **Architect run** or a **Pickup run**: a **Command** started with no **Issue URL**, which decides before any work whether it is skipped. +_Avoid_: tick, cycle + **Claim limit**: The number of open issues carrying a **Claim** at which a **Pickup run** takes no more, set in the **User config**. It keeps a broken **Base branch** from failing every **Ready issue** in turn, and pull requests from piling up unreviewed. _Avoid_: WIP limit, concurrency (it counts issues waiting on the **Day shift**, not Runs in flight) diff --git a/src/architect.rs b/src/architect.rs index df1009199..a05be5b8b 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -29,9 +29,10 @@ use crate::issue::{IssueUrl, Repo}; use crate::labels::{Edit, Label, NEEDS_TRIAGE, READY_FOR_AGENT}; use crate::launch::{self, AlreadyRunning, Launch, Start}; use crate::logs::{self, Pass, Work}; +use crate::pass::{OnGitHub, Outside}; use crate::progress; use crate::prompt; -use crate::ready::{self, ReadyIssue}; +use crate::ready::ReadyIssue; use crate::session::{Logs, Sessions}; use crate::worktree::ReviewWorktree; @@ -225,25 +226,16 @@ pub fn run( } = match launch::start(base)? { Start::Clear(launch) => launch, Start::AlreadyRunning(running) => { - let repo = &running.0; - logs::skipped(Pass::ArchitectRun, repo, &running); - return Ok(Outcome::Skipped(Skipped::AlreadyRunning(running))); + let repo = running.0.clone(); + return Ok(skip(&repo, Skipped::AlreadyRunning(running))); } }; - let open_plans = github::open_issues_labelled(&repo.slug(), ARCHITECT_PLAN)?; - if !open_plans.is_empty() { - return Ok(skip(&repo, Skipped::OpenPlans(open_plans))); - } - let ideas = github::open_issues_labelled(&repo.slug(), ARCHITECT_IDEA)?; - let waiting: Vec<_> = ideas - .into_iter() - .filter(|idea| idea.labels.has(NEEDS_TRIAGE)) - .collect(); - if !waiting.is_empty() { - return Ok(skip(&repo, Skipped::IdeasWaiting(waiting))); - } - if let Some(ReadyIssue { listed, .. }) = ready::first(directory.git(), &repo)? { - return Ok(skip(&repo, Skipped::ReadyIssue(listed))); + let mut on_github = OnGitHub { + launch: directory.git(), + repo: &repo, + }; + if let Decision::Skip(skipped) = gates(&mut on_github)? { + return Ok(skip(&repo, skipped)); } logs::started(Work::ArchitectRun(&repo)); if launch_pull { @@ -262,6 +254,37 @@ pub fn run( }) } +/// What an Architect run's gates decided. +enum Decision { + /// It is skipped, for this reason. + Skip(Skipped), + /// It goes ahead to its review. + GoAhead, +} + +/// The gates of an Architect run, reaching its repository through `pass`: a +/// skip for the open Architect plans, if it has any, then for the +/// Architect ideas still labelled `needs-triage`, then for its Ready issue, +/// by the Pickup run's own search. No gate is read once one skips. +fn gates(pass: &mut impl Outside) -> Result { + let open_plans = pass.open_issues(ARCHITECT_PLAN)?; + if !open_plans.is_empty() { + return Ok(Decision::Skip(Skipped::OpenPlans(open_plans))); + } + let waiting: Vec<_> = pass + .open_issues(ARCHITECT_IDEA)? + .into_iter() + .filter(|idea| idea.labels.has(NEEDS_TRIAGE)) + .collect(); + if !waiting.is_empty() { + return Ok(Decision::Skip(Skipped::IdeasWaiting(waiting))); + } + Ok(match pass.ready_issue()? { + Some(ReadyIssue { listed, .. }) => Decision::Skip(Skipped::ReadyIssue(listed)), + None => Decision::GoAhead, + }) +} + /// The Architect run skipped as `skipped` says, recorded in the Activity log /// of `repo`. fn skip(repo: &Repo, skipped: Skipped) -> Outcome { @@ -423,6 +446,7 @@ fn check_plan(plan: &IssueUrl, origin: &str, started: DateTime) -> Result<( #[cfg(test)] mod tests { use super::*; + use crate::pass::{Call, InMemory}; const URL: &str = "https://github.com/acme/widgets/issues/8"; @@ -459,4 +483,142 @@ mod tests { assert_eq!(Report::read(message), None, "{message:?}"); } } + + /// The gates of an Architect run on `repo`: the reason it is skipped, + /// as its line, or `None` for the go-ahead, the URLs the skip names, + /// and what the gates did. + fn decide(mut repo: InMemory) -> (Option, Vec, Vec) { + let decision = gates(&mut repo).unwrap(); + let (line, urls) = match decision { + Decision::Skip(skipped) => ( + Some(skipped.to_string()), + skipped.urls().into_iter().map(String::from).collect(), + ), + Decision::GoAhead => (None, Vec::new()), + }; + (line, urls, repo.calls) + } + + /// The URL of issue `number` in `acme/widgets`. + fn url(number: u64) -> String { + format!("https://github.com/acme/widgets/issues/{number}") + } + + const PLANS: Call = Call::Open("architect-plan"); + const IDEAS: Call = Call::Open("architect-idea"); + + #[test] + fn with_no_plan_idea_or_ready_issue_the_architect_run_goes_ahead_having_read_each_gate() { + // #7 is a plan a review that failed left half-published, never + // labelled an Architect plan nor an Architect idea. + let repo = InMemory::default() + .issue(3, false, &["architect-plan"]) + .issue(4, false, &["architect-idea", "needs-triage"]) + .issue(7, true, &["needs-triage", "architecture"]); + + let (line, _, calls) = decide(repo); + + assert_eq!(line, None); + assert_eq!(calls, [PLANS, IDEAS, Call::ReadySearch]); + } + + #[test] + fn an_open_plan_skips_naming_each_plan_with_no_read_after() { + let repo = InMemory::default() + .issue(5, true, &["architect-plan"]) + .issue(6, true, &["architect-plan", "ready-for-agent"]) + .issue(7, true, &["architect-idea", "needs-triage"]) + .ready(8, false); + + let (line, urls, calls) = decide(repo); + + assert_eq!( + line.unwrap(), + format!( + "Architect plan #5 \"Issue 5\" is still open: pick it up with thirdshift {}; \ + Architect plan #6 \"Issue 6\" is still open: pick it up with thirdshift {}", + url(5), + url(6) + ) + ); + assert_eq!(urls, [url(5), url(6)]); + assert_eq!(calls, [PLANS]); + } + + #[test] + fn a_waiting_idea_skips_naming_each_waiting_idea_with_no_ready_issue_search() { + let repo = InMemory::default() + .issue(5, true, &["architect-idea", "needs-triage"]) + .issue(6, true, &["architect-idea", "ready-for-agent"]) + .issue(7, true, &["Architect-Idea", "Needs-Triage"]) + .ready(8, false); + + let (line, urls, calls) = decide(repo); + + assert_eq!( + line.unwrap(), + format!( + "Architect idea #5 \"Issue 5\" is waiting for triage: {}; \ + Architect idea #7 \"Issue 7\" is waiting for triage: {}", + url(5), + url(7) + ) + ); + assert_eq!(urls, [url(5), url(7)]); + assert_eq!(calls, [PLANS, IDEAS]); + } + + #[test] + fn an_idea_no_longer_labelled_needs_triage_does_not_skip() { + for decided in ["ready-for-agent", "wontfix", "ready-for-human"] { + let repo = InMemory::default().issue(5, true, &["architect-idea", decided]); + + let (line, _, calls) = decide(repo); + + assert_eq!(line, None, "{decided}"); + assert_eq!(calls, [PLANS, IDEAS, Call::ReadySearch], "{decided}"); + } + } + + #[test] + fn a_ready_issue_skips_naming_it() { + let repo = InMemory::default().ready(9, true); + + let (line, urls, calls) = decide(repo); + + assert_eq!( + line.unwrap(), + format!("Ready issue #9 \"Issue 9\" goes first: {}", url(9)) + ); + assert_eq!(urls, [url(9)]); + assert_eq!(calls, [PLANS, IDEAS, Call::ReadySearch]); + } + + #[test] + fn an_open_plan_wins_over_a_waiting_idea_and_a_ready_issue() { + for idea in [true, false] { + let mut repo = InMemory::default() + .issue(5, true, &["architect-plan"]) + .ready(8, false); + if idea { + repo = repo.issue(6, true, &["architect-idea", "needs-triage"]); + } + + let (_, urls, _) = decide(repo); + + assert_eq!(urls, [url(5)], "with an idea waiting: {idea}"); + } + } + + #[test] + fn a_waiting_idea_wins_over_a_ready_issue() { + let repo = InMemory::default() + .issue(6, true, &["architect-idea", "needs-triage"]) + .ready(8, false); + + let (line, urls, _) = decide(repo); + + assert!(line.unwrap().starts_with("Architect idea #6")); + assert_eq!(urls, [url(6)]); + } } diff --git a/src/claim.rs b/src/claim.rs index e8f392843..e236ab215 100644 --- a/src/claim.rs +++ b/src/claim.rs @@ -8,9 +8,9 @@ use anyhow::{Context, Result}; use crate::branch; use crate::git::Git; -use crate::github::{self, ListedIssue}; +use crate::github; use crate::interrupt; -use crate::issue::{IssueUrl, Repo}; +use crate::issue::IssueUrl; use crate::labels::{self, Edit, Label, Labels, READY_FOR_AGENT}; use crate::progress; @@ -198,40 +198,3 @@ impl Claim<'_> { labels::by_hand(self.issue, off, on) } } - -/// How many open issues in `repo` carry a Claim, whoever started the Run or -/// Spec run that made it: what a Pickup run holds against the Claim limit. -pub fn open_count(repo: &Repo) -> Result { - Ok(github::open_issues_labelled(&repo.slug(), IN_PROGRESS)?.len()) -} - -/// The Sweep: take `in-progress` off every closed issue in `repo` that still -/// carries it, as an issue merged by hand does, leaving its other labels. A -/// failure, to list them or to take the label off one, is only a warning. -pub fn sweep(repo: &Repo) { - let closed = match github::closed_issues_labelled(&repo.slug(), IN_PROGRESS) { - Ok(closed) => closed, - Err(error) => { - progress::step(format_args!( - "warning: could not list the closed issues labelled {IN_PROGRESS}: {error:#}" - )); - return; - } - }; - for ListedIssue { issue, labels, .. } in closed { - let edit = Edit::of(&issue, labels, &[IN_PROGRESS], &[]); - if !edit.takes_off(IN_PROGRESS) { - continue; - } - progress::step(format_args!( - "taking {IN_PROGRESS} off #{}, which is closed", - issue.number - )); - if let Err(error) = edit.apply() { - progress::step(format_args!( - "warning: could not take {IN_PROGRESS} off #{}: {error:#}", - issue.number - )); - } - } -} diff --git a/src/github.rs b/src/github.rs index 88a6b5bad..8e0ec35e5 100644 --- a/src/github.rs +++ b/src/github.rs @@ -263,7 +263,7 @@ pub fn close_issue(issue: &IssueUrl, comment: &str) -> Result<()> { } /// An issue as a listing of its repository's issues gives it. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ListedIssue { pub issue: IssueUrl, pub title: String, diff --git a/src/issue.rs b/src/issue.rs index 2c9c9816d..d6aee56d6 100644 --- a/src/issue.rs +++ b/src/issue.rs @@ -74,7 +74,7 @@ impl IssueUrl { /// The GitHub repository an `origin` remote names, spelled as its URL spells /// it. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Repo { pub owner: String, pub name: String, diff --git a/src/labels.rs b/src/labels.rs index 8353ec670..8340d1d4b 100644 --- a/src/labels.rs +++ b/src/labels.rs @@ -179,6 +179,31 @@ impl<'a> Edit<'a> { &self.labels } + /// The issue it changes. + #[cfg(test)] + pub fn issue(&self) -> &IssueUrl { + self.issue + } + + /// The labels it takes off and puts on, in that order: only those it + /// changes. + #[cfg(test)] + pub fn changes(&self) -> (Vec