From 588d22d2d908bcf16c9090ee589209d1c1dd1561 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 10:04:09 -0400 Subject: [PATCH 1/2] Make Pickup settling wait configurable with a 30-minute default (#573) --- README.md | 16 ++++---- src/asks.rs | 1 + src/config.rs | 20 ++++++++++ src/main.rs | 7 +++- src/pass.rs | 2 +- src/ready.rs | 44 +++++++++++---------- src/setup.rs | 1 + tests/architect_run.rs | 2 +- tests/command_surface.rs | 5 ++- tests/pickup_run.rs | 82 ++++++++++++++++++++++++++++++++++++---- tests/setup.rs | 7 +++- 11 files changed, 146 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 22c88778..dbb00155 100644 --- a/README.md +++ b/README.md @@ -201,6 +201,7 @@ parallel = 2 # how many Tickets a Spec run runs at once, instead of 3 [pickup] limit = 5 # how many open issues labelled in-progress stop a Pickup run taking another, instead of 3 +wait_minutes = 60 # minutes since the latest shaping event before an issue is Ready, instead of 30; 0 disables waiting [harness] default = "claude" # the Harness every session runs on; claude unless set @@ -246,6 +247,7 @@ parallel = 3 # how many Tickets a Spec run runs at once; default 3 [pickup] limit = 3 # how many open issues labelled in-progress stop a Pickup run taking another; default 3 +wait_minutes = 30 # minutes since the latest shaping event before an issue is Ready; 0 disables waiting; default 30 [harness] default = "claude" # the Harness every Run's sessions run on, claude, codex, agy, grok, muse or opencode; default claude @@ -646,7 +648,7 @@ Work a human shaped goes ahead of the factory's own: while the repository has a - **stderr** carries, as a Pickup run's does, [a line on each issue labelled `ready-for-agent`](#why-an-issue-was-passed-over) passed over before it, then one progress line naming it: `Ready issue # "" goes first: <Issue URL>`. - **Exit code** `0`: skipped is not a failure. No agent session starts, and nothing else is done. -An issue labelled `ready-for-agent` that is not a Ready issue never skips an Architect run: one that carries a Claim, was labelled or shaped less than ten minutes ago, has an open blocker, is a Ticket inside a Spec that is not itself a Ready issue, or is a Base fix's issue, among the rest of the rule. Those get their lines on stderr all the same, and the Architect run goes on. +An issue labelled `ready-for-agent` that is not a Ready issue never skips an Architect run: one that carries a Claim, was labelled or shaped within the configured settling window (thirty minutes by default), has an open blocker, is a Ticket inside a Spec that is not itself a Ready issue, or is a Base fix's issue, among the rest of the rule. Those get their lines on stderr all the same, and the Architect run goes on. The rule doesn't ask whether anything will take the Ready issue. A `ready-for-agent` issue that you mean to run by hand, on a repository with no Pickup line in its crontab, keeps every Architect run on it from starting until you run it, which makes the Claim, or take the label off. @@ -748,7 +750,7 @@ A Ready issue is an open issue that: - has no open blocker, by GitHub's "blocked by" links, never the text of its body. Once every blocker is closed, it can be taken; - was never started: no **Issue branch** for it is on `origin`, and no pull request from one exists, open, merged or closed. So a Pickup run never does a [Continuation](#continuation), and an issue whose Run failed waits for you; - is not a **Spec** whose **Tickets** are all closed. With nothing started on it, such a Spec was done some other way, and the [Spec run](#spec-runs) it would be dispatched as stops with nothing to do, so a Pickup run passes it over rather than take it on every pass. Waiting never makes it a Ready issue, so this comes before the next condition, however lately the Spec was labelled: close it, or take its `ready-for-agent` off. A Spec with at least one open Ticket is taken; -- is settled: ten minutes have passed since the latest of `ready-for-agent` being applied to it, a sub-issue being added to it or removed, and a "blocked by" link being added to it or removed. So a Spec is not taken while its Tickets are still being attached. These are read from the issue's timeline, since adding a sub-issue does not change the issue's update time, and the ten minutes is fixed, with no setting. A later pass takes the issue once it has settled. +- is settled: by default, thirty minutes have passed since the latest of `ready-for-agent` being applied to it, a sub-issue being added to it or removed, and a "blocked by" link being added to it or removed. So a Spec is not taken while its Tickets are still being attached. These are read from the issue's timeline, since adding a sub-issue does not change the issue's update time, and `pickup.wait_minutes` in the [User config](#user-config) sets the window in whole minutes from 0 up, within the supported duration range; 0 disables waiting. The setting also controls this same Ready issue search for Architect runs. A later pass takes the issue once it has settled. A Pickup run: @@ -799,9 +801,9 @@ thirdshift: 03:00:04 #30 blocked by #29 thirdshift: 03:00:05 #31 already started: issue-31 is on origin thirdshift: 03:00:06 #32 already started: PR https://github.com/acme/widgets/pull/37 thirdshift: 03:00:06 #33 every Ticket is closed -thirdshift: 03:00:07 #34 not settled: labelled ready-for-agent less than 10 minutes ago -thirdshift: 03:00:07 #35 not settled: a sub-issue added or removed less than 10 minutes ago -thirdshift: 03:00:08 #36 not settled: a "blocked by" link added or removed less than 10 minutes ago +thirdshift: 03:00:07 #34 not settled: labelled ready-for-agent less than 30 minutes ago +thirdshift: 03:00:07 #35 not settled: a sub-issue added or removed less than 30 minutes ago +thirdshift: 03:00:08 #36 not settled: a "blocked by" link added or removed less than 30 minutes ago thirdshift: 03:00:08 no Ready issue on acme/widgets ``` @@ -839,7 +841,7 @@ PATH=/home/you/.local/bin:/home/you/.cargo/bin:/usr/local/bin:/usr/bin:/bin The `PATH` line, the `cd` and `base main`, the log file's directory, the logins `claude` and `gh` need, and schedulers other than cron are as for an Architect run: see its [On a schedule](#on-a-schedule). What differs for a Pickup run: -- **The interval** sets how soon a Ready issue is taken and how much work is started: a pass takes one issue, so `*/30` starts at most one every half hour. A pass lasts as long as the run it dispatched, and the passes that fire meanwhile are skipped, so the schedule builds one issue of a repository at a time on a machine, and the first pass after it ends takes the next, unless the repository is by then at its [Claim limit](#the-claim-limit): each pull request left for review, and each failure left for you, holds a Claim, and at 3 of them, or the `pickup.limit` you set, the passes take nothing until you have dealt with one. An issue you have just labelled also waits ten minutes, until it is settled. A Run you start by hand on an Issue URL is outside all this: it neither waits for a pass nor makes one skip. +- **The interval** sets how soon a Ready issue is taken and how much work is started: a pass takes one issue, so `*/30` starts at most one every half hour. A pass lasts as long as the run it dispatched, and the passes that fire meanwhile are skipped, so the schedule builds one issue of a repository at a time on a machine, and the first pass after it ends takes the next, unless the repository is by then at its [Claim limit](#the-claim-limit): each pull request left for review, and each failure left for you, holds a Claim, and at 3 of them, or the `pickup.limit` you set, the passes take nothing until you have dealt with one. An issue you have just labelled also waits thirty minutes by default, or the `pickup.wait_minutes` you set, until it is settled. A Run you start by hand on an Issue URL is outside all this: it neither waits for a pass nor makes one skip. - **One line per repository.** thirdshift keeps no list of repositories. A Pickup run and an Architect run on one repository each skip while the other is still running, the Spec run or Run it dispatched included, so both lines can go in one crontab and the two never build that repository at once. Passes on different repositories do run at the same time: give their lines different minutes, such as `15,45`, if they would compete for the machine. - **A pass is skipped** when the [lock is held](#one-at-a-time), when the repository has no Ready issue, and when it is at its Claim limit. A skip exits `0`, so the scheduler sees no failure, and sends no email, even with Run notifications on. The repository's [Activity log](#logs) records a skip when its reason changes. Unless `activity.quiet_skips` is set, the log file holds each skip's line too, and before the line of a pass that found no Ready issue, as before the line of one that took an issue, [why each issue was passed over](#why-an-issue-was-passed-over). A pass skipped for the lock or the Claim limit looks at no issue, so it has no such lines. - **A Run notification**, by `email.always = true` in the [User config](#user-config) or `email` on the line, is sent [only for an issue taken](#one-run-notification-for-the-issue-taken). So a day without email doesn't say the passes are running: a pass that fails before it takes an issue, on a broken User config, a missing Resend key or a failed check, shows up only in the log file. @@ -867,7 +869,7 @@ The command's flags and the User config decide what a pass does with the issue i An issue whose run failed [keeps its Claim](#when-the-claim-ends) and waits for the **Day shift**: it stays `in-progress`, with the Issue branch and any draft pull request the run left, no later pass takes it, and it counts towards the Claim limit until it is closed or you take the label off. To send it round again by hand, run `thirdshift <Issue URL>` from the clone, with the Base branch checked out, since that command takes no `base <branch>`: it picks up where the failed run stopped, as a [Continuation](#continuation) does. Labelling it `ready-for-agent` again doesn't do it: a Pickup run never takes an issue that was started. The one failure a later pass does retry is one that left nothing on `origin`, such as a usage limit or an expired login: its Claim is released, so the issue is a Ready issue again once it has settled. -If you shape your issues with the upstream `to-spec` and `to-tickets`, from [mattpocock/skills](https://github.com/mattpocock/skills), unchanged: `ready-for-agent` on a **Spec** must mean its **Tickets** are published, every Ticket attached as a sub-issue and every "blocked by" link in place. Upstream, `to-spec` labels the Spec `ready-for-agent` when it publishes it, minutes or days before `to-tickets` attaches the Tickets, and a Spec with no Tickets yet looks exactly like a standalone Ticket, so a pass would start a plain Run on it. Label such a Spec `needs-triage` until its Tickets are attached, then swap that for `ready-for-agent`, as the copies of the two this repository's own Day shift uses, under `.agents/skills/`, do ([`docs/agents/triage-labels.md`](docs/agents/triage-labels.md)). The ten minutes an issue must be settled for are only a second line of defence: they cover Tickets attached within minutes of the label, not a Spec left labelled and without Tickets for longer. +If you shape your issues with the upstream `to-spec` and `to-tickets`, from [mattpocock/skills](https://github.com/mattpocock/skills), unchanged: `ready-for-agent` on a **Spec** must mean its **Tickets** are published, every Ticket attached as a sub-issue and every "blocked by" link in place. Upstream, `to-spec` labels the Spec `ready-for-agent` when it publishes it, minutes or days before `to-tickets` attaches the Tickets, and a Spec with no Tickets yet looks exactly like a standalone Ticket, so a pass would start a plain Run on it. Label such a Spec `needs-triage` until its Tickets are attached, then swap that for `ready-for-agent`, as the copies of the two this repository's own Day shift uses, under `.agents/skills/`, do ([`docs/agents/triage-labels.md`](docs/agents/triage-labels.md)). The settling window (thirty minutes by default) is only a second line of defence: it covers Tickets attached within minutes of the label, not a Spec left labelled and without Tickets for longer. ## Building from source diff --git a/src/asks.rs b/src/asks.rs index c03951dd..d70c0c1e 100644 --- a/src/asks.rs +++ b/src/asks.rs @@ -242,6 +242,7 @@ mod tests { email: EmailSettings::default(), spec_parallel: n(3), pickup_limit: n(3), + pickup_wait: chrono::TimeDelta::minutes(30), harness: harness::Settings::default(), } } diff --git a/src/config.rs b/src/config.rs index 148a6dae..4f978ec6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -8,6 +8,7 @@ use std::num::NonZeroUsize; use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; +use chrono::TimeDelta; use toml::{Table, Value}; use toml_edit::{DocumentMut, Item}; @@ -39,6 +40,10 @@ pub struct UserConfig { /// `pickup.limit`: the Claim limit, how many open issues carrying a /// Claim stop a Pickup run from taking another, by default 3. pub pickup_limit: NonZeroUsize, + /// `pickup.wait_minutes`: how long after its latest shaping event an + /// issue becomes a Ready issue, by default 30 minutes. Zero disables + /// the settling window. + pub pickup_wait: TimeDelta, /// The `[harness]` section: the Harness every Command runs its sessions /// on, and a Model and Effort for each Harness. pub harness: harness::Settings, @@ -82,6 +87,7 @@ impl UserConfig { email: EmailSettings::default(), spec_parallel: NonZeroUsize::new(3).unwrap(), pickup_limit: NonZeroUsize::new(3).unwrap(), + pickup_wait: TimeDelta::minutes(30), harness: harness::Settings::default(), } } @@ -151,6 +157,18 @@ impl UserConfig { ("pickup", "limit", value) => { config.pickup_limit = whole_number_from_1(value, "pickup.limit", &file)? } + ("pickup", "wait_minutes", value) => { + config.pickup_wait = value + .as_integer() + .filter(|minutes| *minutes >= 0) + .and_then(TimeDelta::try_minutes) + .with_context(|| { + format!( + "pickup.wait_minutes must be a whole number of minutes from 0 up \ + within the supported range in {file}" + ) + })?; + } ("harness", "default", Value::String(name)) => match Harness::named(name) { Some(harness) => config.harness.default = Some(harness), None => bail!( @@ -649,6 +667,7 @@ parallel = 3 # how many Tickets a Spec run runs at once; default 3 [pickup] limit = 3 # how many open issues labelled in-progress stop a Pickup run taking another; default 3 +wait_minutes = 30 # minutes since the latest shaping event before an issue is Ready; 0 disables waiting; default 30 [harness] default = "claude" # the Harness every Run's sessions run on, claude, codex, agy, grok, muse or opencode; default claude @@ -803,6 +822,7 @@ mod tests { "activity.quiet_skips", "spec.parallel", "pickup.limit", + "pickup.wait_minutes", "harness.default", "harness.claude.model", "harness.claude.effort", diff --git a/src/main.rs b/src/main.rs index 11486b7c..85308ba9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -247,10 +247,15 @@ ready-for-agent that has none of ready-for-human, needs-info, wontfix and needs- not in-progress, is not a sub-issue, is not labelled base-fix, has no open blocker, and was never started: no Issue branch for it is on origin, and no pull request from one exists, open, merged or closed. A Spec whose Tickets are all closed is not one either: a Spec run -would find nothing to do. It must also be settled: ten minutes have passed since +would find nothing to do. It must also be settled: by default, thirty minutes have passed since ready-for-agent was applied to it, and since a sub-issue or a \"blocked by\" link of its was last added or removed, so a Spec is not taken while its Tickets are being attached. A sub-issue is reached through its Spec, when the Spec is itself a Ready issue. +pickup.wait_minutes in the User config sets that settling window in whole minutes from +0 up, within the supported duration range; 0 disables waiting. There is no flag for it: + + [pickup] + wait_minutes = 30 Each ready-for-agent issue a pass looks at and does not take gets one line on stderr with the first reason that applies, such as #21 is a Ticket of #20, which is not ready or #30 diff --git a/src/pass.rs b/src/pass.rs index f74bf5d8..71a7af6d 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -134,7 +134,7 @@ impl Outside for LaunchAndGitHub<'_> { } fn ready_issue(&mut self) -> Result<Option<ReadyIssue>> { - ready::first(self.launch.git(), self.repo) + ready::first(self.launch.git(), self.repo, self.config.pickup_wait) } fn step(&mut self, line: String) { diff --git a/src/ready.rs b/src/ready.rs index fbfe5164..5dfa4621 100644 --- a/src/ready.rs +++ b/src/ready.rs @@ -28,7 +28,9 @@ pub struct ReadyIssue { /// `launch`'s repository, if there is one, with a line on stderr for each /// issue labelled `ready-for-agent` passed over before it, with why. /// Nothing is changed, on GitHub or in `launch`. -pub fn first(launch: &Git, repo: &Repo) -> Result<Option<ReadyIssue>> { +/// `wait` is the User config's settling window since the latest shaping +/// event; every pass uses the same Ready issue definition. +pub fn first(launch: &Git, repo: &Repo, wait: TimeDelta) -> Result<Option<ReadyIssue>> { let reads = GitHubAndOrigin { launch, github: GitHub::new(), @@ -36,7 +38,7 @@ pub fn first(launch: &Git, repo: &Repo) -> Result<Option<ReadyIssue>> { let candidates = reads .github .open_issues_labelled(&repo.slug(), READY_FOR_AGENT)?; - Search::new(&reads, candidates, Utc::now(), progress::step).first_ready() + Search::new(&reads, candidates, Utc::now(), wait, progress::step).first_ready() } /// What the search reads of an issue beyond its listing, each only when a @@ -65,11 +67,6 @@ impl Reads for GitHubAndOrigin<'_> { } } -/// How long an issue is left after it was last shaped before it is a Ready -/// issue, so a Spec is not taken while its Tickets are still being -/// attached. Fixed, with no setting. -const SETTLE: TimeDelta = TimeDelta::minutes(10); - /// Where an open issue labelled `ready-for-agent` stands in the search. #[derive(Clone)] enum Standing { @@ -100,8 +97,8 @@ enum Reason { /// It is a Spec whose Tickets are all closed, with nothing started: the /// Spec run it would be dispatched as refuses it, having nothing to do. TicketsClosed, - /// It is not settled: it was last shaped, by this, less than [`SETTLE`] - /// ago. + /// It is not settled: it was last shaped, by this, within the + /// configured settling window. Unsettled(Shaping), } @@ -116,6 +113,9 @@ struct Search<'a, R, P> { standings: Vec<Option<Standing>>, /// The time of the pass, which settling is measured to. now: DateTime<Utc>, + /// How long an issue is left after it was last shaped before it is a + /// Ready issue, so a Spec is not taken while Tickets are attached. + wait: TimeDelta, /// Where each line on an issue passed over goes, as soon as it is. passed_over: P, } @@ -125,6 +125,7 @@ impl<'a, R: Reads, P: FnMut(String)> Search<'a, R, P> { reads: &'a R, mut candidates: Vec<ListedIssue>, now: DateTime<Utc>, + wait: TimeDelta, passed_over: P, ) -> Self { candidates.sort_by_key(|candidate| candidate.issue.number); @@ -134,6 +135,7 @@ impl<'a, R: Reads, P: FnMut(String)> Search<'a, R, P> { candidates, standings, now, + wait, passed_over, } } @@ -162,7 +164,7 @@ impl<'a, R: Reads, P: FnMut(String)> Search<'a, R, P> { if let Some(standing) = &self.standings[at] { return Ok(standing.clone()); } - let standing = standing_of(self.reads, &self.candidates[at], self.now)?; + let standing = standing_of(self.reads, &self.candidates[at], self.now, self.wait)?; self.standings[at] = Some(standing.clone()); Ok(standing) } @@ -212,7 +214,7 @@ impl<'a, R: Reads, P: FnMut(String)> Search<'a, R, P> { Shaping::SubIssues => "a sub-issue added or removed".to_string(), Shaping::Blockers => "a \"blocked by\" link added or removed".to_string(), }; - let minutes = SETTLE.num_minutes(); + let minutes = self.wait.num_minutes(); format!("not settled: {shaped} less than {minutes} minutes ago") } }; @@ -225,13 +227,14 @@ impl<'a, R: Reads, P: FnMut(String)> Search<'a, R, P> { /// Ready issue when it has no label that makes an Unready Ticket and no /// Claim, is not a sub-issue, has no `base-fix` label and no open blocker, /// was never started, is not a Spec whose Tickets are all closed, and is -/// settled: [`SETTLE`] has passed since it was last labelled +/// settled: `wait` has passed since it was last labelled /// `ready-for-agent` and since a sub-issue or a "blocked by" link of its was /// last added or removed. fn standing_of( reads: &impl Reads, candidate: &ListedIssue, now: DateTime<Utc>, + wait: TimeDelta, ) -> Result<Standing> { let passed_over = |reason| Ok(Standing::PassedOver(reason)); if let Some(label) = candidate.labels.unready() { @@ -257,7 +260,7 @@ fn standing_of( return passed_over(Reason::TicketsClosed); } if let Some(shaped) = read.last_shaped - && now - shaped.at < SETTLE + && now - shaped.at < wait { return passed_over(Reason::Unsettled(shaped.by)); } @@ -386,7 +389,7 @@ mod tests { seen: &seen, }; let sink = |line| seen.borrow_mut().push(Seen::Line(line)); - let ready = Search::new(&reads, candidates, now(), sink) + let ready = Search::new(&reads, candidates, now(), TimeDelta::minutes(30), sink) .first_ready() .unwrap() .map(|ready| (ready.listed.issue.number, ready.is_spec)); @@ -633,15 +636,16 @@ mod tests { } #[test] - fn an_issue_settles_ten_minutes_after_it_was_last_shaped_whatever_shaped_it() { - let just_under = SETTLE - TimeDelta::seconds(1); - let just_over = SETTLE + TimeDelta::seconds(1); + fn an_issue_settles_thirty_minutes_after_it_was_last_shaped_whatever_shaped_it() { + let wait = TimeDelta::minutes(30); + let just_under = wait - TimeDelta::seconds(1); + let just_over = wait + TimeDelta::seconds(1); for (by, said) in [ (Shaping::Labelled, "labelled ready-for-agent"), (Shaping::SubIssues, "a sub-issue added or removed"), (Shaping::Blockers, "a \"blocked by\" link added or removed"), ] { - for (ago, settled) in [(just_under, false), (SETTLE, true), (just_over, true)] { + for (ago, settled) in [(just_under, false), (wait, true), (just_over, true)] { let facts = Facts { last_shaped: shaped(by, ago), ..Facts::default() @@ -655,7 +659,7 @@ mod tests { } else { assert_eq!(ready, None, "{said}, {ago}"); let line = - passed_over(&format!("#7 not settled: {said} less than 10 minutes ago")); + passed_over(&format!("#7 not settled: {said} less than 30 minutes ago")); assert_eq!(seen, [candidate, started, line], "{said}, {ago}"); } } @@ -709,7 +713,7 @@ mod tests { ( &[], facts(false, false, false, false), - "#7 not settled: labelled ready-for-agent less than 10 minutes ago", + "#7 not settled: labelled ready-for-agent less than 30 minutes ago", ), ] { let (ready, seen) = search(vec![listed(7, labels)], vec![(7, facts)]); diff --git a/src/setup.rs b/src/setup.rs index 10b42049..1819bb0e 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -1124,6 +1124,7 @@ parallel = 5 [pickup] limit = 5 +wait_minutes = 60 [harness] default = \"claude\" diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 46b0e186..ef75a8e4 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -2334,7 +2334,7 @@ fn a_ready_for_agent_issue_that_is_claimed_unsettled_blocked_or_a_base_fixs_does let labelled = (TimelineEvent::Labelled("ready-for-agent"), 9); scenario.issue_timeline(7, &[labelled]); }, - "#7 not settled: labelled ready-for-agent less than 10 minutes ago".to_string(), + "#7 not settled: labelled ready-for-agent less than 30 minutes ago".to_string(), ), ( |scenario| scenario.issue_blocked_by(7, &[5]), diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 44239fc9..d3bb2d14 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -225,7 +225,10 @@ fn help_documents_pickup_the_ready_issue_the_dispatch_the_skips_and_the_run_noti "is not a sub-issue, is not labelled base-fix, has no open blocker", "was never started: no Issue branch for it is on origin, and no pull request from one exists, open, merged or closed", "A Spec whose Tickets are all closed is not one either: a Spec run would find nothing to do", - "It must also be settled: ten minutes have passed since ready-for-agent was applied to it, and since a sub-issue or a \"blocked by\" link of its was last added or removed", + "It must also be settled: by default, thirty minutes have passed since ready-for-agent was applied to it, and since a sub-issue or a \"blocked by\" link of its was last added or removed", + "pickup.wait_minutes in the User config sets that settling window in whole minutes from 0 up", + "0 disables waiting", + "[pickup] wait_minutes = 30", "A sub-issue is reached through its Spec, when the Spec is itself a Ready issue", "Each ready-for-agent issue a pass looks at and does not take gets one line on stderr with the first reason that applies, such as #21 is a Ticket of #20, which is not ready or #30 blocked by #29, before the line that says what the pass did", "The Pickup run ends as that run does, with its exit code and its PR's URL", diff --git a/tests/pickup_run.rs b/tests/pickup_run.rs index 2cda8722..f64fffbb 100644 --- a/tests/pickup_run.rs +++ b/tests/pickup_run.rs @@ -290,10 +290,10 @@ fn an_issue_with_an_open_blocker_is_not_taken_and_with_that_blocker_closed_it_is /// What a Pickup run says of #7 when it was labelled `ready-for-agent` too /// recently to have settled. const LABELLED_TOO_RECENTLY: &str = - "#7 not settled: labelled ready-for-agent less than 10 minutes ago"; + "#7 not settled: labelled ready-for-agent less than 30 minutes ago"; #[test] -fn an_issue_labelled_ready_for_agent_less_than_ten_minutes_ago_is_not_taken() { +fn an_issue_labelled_ready_for_agent_less_than_thirty_minutes_ago_is_not_taken() { let scenario = ready_ticket(); // Its other labels, however recent, and an earlier `ready-for-agent` // don't count: only the last time `ready-for-agent` was applied does. @@ -301,7 +301,7 @@ fn an_issue_labelled_ready_for_agent_less_than_ten_minutes_ago_is_not_taken() { 7, &[ (TimelineEvent::Labelled(READY_FOR_AGENT), 60), - (TimelineEvent::Labelled(READY_FOR_AGENT), 9), + (TimelineEvent::Labelled(READY_FOR_AGENT), 29), (TimelineEvent::Labelled("bug"), 1), ], ); @@ -314,12 +314,12 @@ fn an_issue_labelled_ready_for_agent_less_than_ten_minutes_ago_is_not_taken() { } #[test] -fn an_issue_labelled_ready_for_agent_more_than_ten_minutes_ago_is_taken() { +fn an_issue_labelled_ready_for_agent_more_than_thirty_minutes_ago_is_taken() { let scenario = ready_ticket(); scenario.issue_timeline( 7, &[ - (TimelineEvent::Labelled(READY_FOR_AGENT), 11), + (TimelineEvent::Labelled(READY_FOR_AGENT), 31), (TimelineEvent::Labelled("bug"), 1), ], ); @@ -330,7 +330,52 @@ fn an_issue_labelled_ready_for_agent_more_than_ten_minutes_ago_is_taken() { } #[test] -fn a_spec_labelled_long_ago_whose_sub_issues_or_blockers_changed_less_than_ten_minutes_ago_is_not_taken() +fn pickup_wait_in_the_user_config_shortens_lengthens_or_disables_settling() { + for (wait, ago, taken) in [(5, 6, true), (60, 31, false), (0, 0, true)] { + for (event, shaped) in [ + ( + TimelineEvent::Labelled(READY_FOR_AGENT), + "labelled ready-for-agent", + ), + (TimelineEvent::SubIssueAdded, "a sub-issue added or removed"), + ( + TimelineEvent::SubIssueRemoved, + "a sub-issue added or removed", + ), + ( + TimelineEvent::BlockedByAdded, + "a \"blocked by\" link added or removed", + ), + ( + TimelineEvent::BlockedByRemoved, + "a \"blocked by\" link added or removed", + ), + ] { + let scenario = ready_ticket(); + scenario.user_config_is(&format!("[pickup]\nwait_minutes = {wait}\n")); + scenario.issue_timeline( + 7, + &[ + (TimelineEvent::Labelled(READY_FOR_AGENT), 600), + (event, ago), + ], + ); + + let result = scenario.run(&["pickup"]); + + if taken { + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); + } else { + let line = format!("#7 not settled: {shaped} less than {wait} minutes ago"); + assert_skipped(&scenario, &result, &no_ready_issue_after(&[&line])); + assert_eq!(scenario.issue_labels(7), [READY_FOR_AGENT]); + } + } + } +} + +#[test] +fn a_spec_labelled_long_ago_whose_sub_issues_or_blockers_changed_less_than_thirty_minutes_ago_is_not_taken() { for (event, changed) in [ (TimelineEvent::SubIssueAdded, "a sub-issue"), @@ -344,13 +389,13 @@ fn a_spec_labelled_long_ago_whose_sub_issues_or_blockers_changed_less_than_ten_m &[ (TimelineEvent::Labelled(READY_FOR_AGENT), 600), (TimelineEvent::SubIssueAdded, 590), - (event, 5), + (event, 29), ], ); let result = scenario.run(&["pickup"]); - let line = format!("#7 not settled: {changed} added or removed less than 10 minutes ago"); + let line = format!("#7 not settled: {changed} added or removed less than 30 minutes ago"); assert_skipped(&scenario, &result, &no_ready_issue_after(&[&line])); assert_eq!(scenario.issue_labels(7), [READY_FOR_AGENT]); } @@ -1286,6 +1331,27 @@ fn a_pickup_limit_that_is_not_a_whole_number_from_1_up_stops_the_pass_naming_the } } +#[test] +fn an_invalid_pickup_wait_stops_the_pass_naming_the_file_and_key() { + for wait in ["-1", "1.5", "\"30\"", "true", "9223372036854775807"] { + let scenario = ready_ticket(); + let path = scenario.user_config_is(&format!("[pickup]\nwait_minutes = {wait}\n")); + + let result = scenario.run(&["pickup"]); + + scenario.assert_rejected_before_any_work( + &result, + &format!( + "thirdshift: pickup.wait_minutes must be a whole number of minutes from 0 up \ + within the supported range in the User config {}\n", + path.display() + ), + ); + assert_eq!(result.code, Some(1), "{wait}: {}", result.stderr); + assert_eq!(scenario.gh_calls(), Vec::<Vec<String>>::new()); + } +} + /// Make issue `number` closed on the fake GitHub, still labelled /// `in-progress`, after the labels `before` it: an issue merged by hand, /// which no Self-merge took the Claim off. diff --git a/tests/setup.rs b/tests/setup.rs index 94b4d0da..91548450 100644 --- a/tests/setup.rs +++ b/tests/setup.rs @@ -74,6 +74,7 @@ fn with_no_terminal_and_no_user_config_setup_writes_the_defaults() { assert_eq!(config["activity"]["quiet_skips"].as_bool(), Some(false)); assert_eq!(config["spec"]["parallel"].as_integer(), Some(3)); assert_eq!(config["pickup"]["limit"].as_integer(), Some(3)); + assert_eq!(config["pickup"]["wait_minutes"].as_integer(), Some(30)); assert_eq!(config["harness"]["default"].as_str(), Some("claude")); for harness in ["claude", "codex", "agy", "grok", "muse", "opencode"] { for key in ["model", "effort"] { @@ -91,8 +92,9 @@ fn with_no_terminal_and_no_user_config_setup_writes_the_defaults() { fn completing_an_existing_file_replaces_it_atomically_keeps_permissions_and_then_avoids_replacement() { let scenario = Scenario::new(); - let original = - "# my machine\n[logs]\ndir = '/var/log/ts' # custom\n[merge]\nalways = true # merge\n"; + let original = "# my machine\n[logs]\ndir = '/var/log/ts' # custom\n\ + [merge]\nalways = true # merge\n\ + [pickup]\nwait_minutes = 60 # time to attach Tickets\n"; let path = scenario.user_config_is(original); fs::set_permissions(&path, fs::Permissions::from_mode(0o640)).unwrap(); let mut previous_file = fs::File::open(&path).unwrap(); @@ -105,6 +107,7 @@ fn completing_an_existing_file_replaces_it_atomically_keeps_permissions_and_then assert!(text.starts_with(original), "{text}"); let config: toml::Table = text.parse().unwrap(); assert_eq!(config["logs"]["dir"].as_str(), Some("/var/log/ts")); + assert_eq!(config["pickup"]["wait_minutes"].as_integer(), Some(60)); assert_eq!(config["harness"]["default"].as_str(), Some("claude")); let metadata = fs::metadata(&path).unwrap(); assert_eq!(metadata.permissions().mode() & 0o777, 0o640); From 35833c776838d316cd37384f4e97444b9efe2056 Mon Sep 17 00:00:00 2001 From: Jacob Stephens <jacob@stephens.page> Date: Thu, 8 Oct 2026 10:09:39 -0400 Subject: [PATCH 2/2] Honor zero Pickup wait even with future timeline timestamps --- src/ready.rs | 3 ++- tests/pickup_run.rs | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/ready.rs b/src/ready.rs index 5dfa4621..f4a257a2 100644 --- a/src/ready.rs +++ b/src/ready.rs @@ -259,7 +259,8 @@ fn standing_of( if spec_run::all_closed(read.sub_issue_is_open.iter().copied()) { return passed_over(Reason::TicketsClosed); } - if let Some(shaped) = read.last_shaped + if wait > TimeDelta::zero() + && let Some(shaped) = read.last_shaped && now - shaped.at < wait { return passed_over(Reason::Unsettled(shaped.by)); diff --git a/tests/pickup_run.rs b/tests/pickup_run.rs index f64fffbb..7383e944 100644 --- a/tests/pickup_run.rs +++ b/tests/pickup_run.rs @@ -374,6 +374,23 @@ fn pickup_wait_in_the_user_config_shortens_lengthens_or_disables_settling() { } } +#[test] +fn zero_pickup_wait_ignores_server_timestamps_ahead_of_local_clock() { + let scenario = ready_ticket(); + scenario.user_config_is("[pickup]\nwait_minutes = 0\n"); + scenario.issue_timeline(7, &[(TimelineEvent::Labelled(READY_FOR_AGENT), -1)]); + + let result = scenario.run(&["pickup"]); + + assert_eq!( + scenario.claude_calls().len(), + 1, + "Pickup did not dispatch with zero wait:\n{}", + result.stderr + ); + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); +} + #[test] fn a_spec_labelled_long_ago_whose_sub_issues_or_blockers_changed_less_than_thirty_minutes_ago_is_not_taken() {