diff --git a/README.md b/README.md index e5f9aeb1c..7fc201b4f 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ From a clone of the issue's repository, `thirdshift `: - a local Issue branch, if you have one, points at the same commit as its copy on `origin`, so local-only commits are never destroyed. Any failed check stops the Run before any work happens. -2. Picks the **Issue branch** (`issue-`, or `issue--branch-` once earlier ones are finished) and the **Base branch**, normally the branch you have checked out. +2. Picks the **Issue branch** (`issue-`, or `issue--branch-` once earlier ones are finished) and the **Base branch**, normally the branch you have checked out. Then it makes the [Claim](#the-claim): it labels the issue `in-progress`, in place of `ready-for-agent` if it has that. 3. Creates a git worktree next to your clone, named `-`, so your own checkout is never touched. 4. Runs a headless Claude Code session in that worktree with the **Factory skills** in [`skills/`](skills/) loaded, started with one of the **Session prompts** in [`prompts/`](prompts/). The agent implements the issue, reviews its work against the Base branch, addresses the **Standards findings** and **Spec findings** it agrees with, and opens a ready-for-review pull request that lists every **Unaddressed finding** with a reason and closes the issue. 5. Takes over deterministically: pushes the Issue branch, skipping your repo's git hooks since the session runs the tests and CI gates the pull request, checks through `gh` that the pull request exists, is open and targets the Base branch, and marks it ready for review (`gh pr ready`) if the agent left it as a draft. @@ -25,6 +25,20 @@ From a clone of the issue's repository, `thirdshift `: 7. In a **Merge run** (`thirdshift merge `), does the **Self-merge**: once the pull request is open, ready for review, mergeable and green, thirdshift merges it into the Base branch with a merge commit, on exactly the head commit whose CI it watched (`gh pr merge --merge --match-head-commit `). It never uses GitHub's auto-merge ([ADR-0004](docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md)). A merge that fails goes back round step 6, within the same budgets, and is tried again on the new green head. If that round finds nothing to fix, the refusal is a **policy refusal**, such as merge commits being disallowed or a review being required. After the merge, thirdshift deletes the Issue branch on `origin`, and closes the issue if it is still open, with the comment `Closed by #, merged into by a thirdshift Merge run.` GitHub closes it on its own only for a merge into the repository's default branch, and then thirdshift leaves it alone. 8. Cleans up: removes the worktree, the local Issue branch and the temporary plugin directory, whether the Run succeeded or not. The one exception is a **Failed run** whose work could not be pushed: see below. +### The Claim + +The **Claim** is the mark that the factory has taken an issue, so the issue list shows what it is working on. When a Run or a [Spec run](#spec-runs) starts on an issue, thirdshift labels the issue `in-progress`, in place of `ready-for-agent` if it has that. An issue with no `ready-for-agent` label still gets `in-progress`, so the label means the same thing however the Run was started. + +- It is made once every check of step 1 has passed and the branches are picked, and before the worktree is created. A Run stopped by a check changes no label. +- It is one request, which adds `in-progress`, removes `ready-for-agent` and keeps the issue's other labels, so the swap can't stop halfway. stderr says `labelling # in-progress, in place of ready-for-agent`, or `labelling # in-progress`. +- thirdshift first creates the `in-progress` label if the repository has none. +- It is made however the Run or Spec run was started: by you on an Issue URL, or by an [Architect run](#architect-runs) for the plan it dispatches. +- A Ticket's Run in a Spec run makes none, since the Spec carries the Claim, and nor does a [Base fix](#base-fix): their issues' labels are left as they are. +- A [Continuation](#continuation) on an issue that is already `in-progress` changes nothing and makes no request. One on an issue that isn't Claimed yet Claims it. +- If the Claim can't be made, the Run stops there, before any work, with exit `1` and the cause: `could not make the Claim on #: `. + +thirdshift doesn't take `in-progress` off again yet: the label stays when the Run ends, whatever its outcome. + ### Status The designed behaviour described in this README is implemented. Known gaps and planned work are tracked in the [open issues](https://github.com/JacobStephens2/thirdshift/issues). @@ -309,7 +323,7 @@ When the Run's only red checks are Inherited failures and the Base branch has no 1. thirdshift looks for an open issue labelled `base-fix`, other than the Run's own, whose title names the same Base branch and every one of those checks. If there is one, another Run's Base fix is already under way, and the Run waits on that one instead: see [One Base fix per Base branch](#one-base-fix-per-base-branch). 2. Otherwise, thirdshift opens an issue from a fixed template, with no agent session: titled `CI red on : [, …]`, its body naming the checks with their URLs on the Base branch, the Base branch and its short sha, and the Run's pull request, labelled `base-fix` and `ready-for-agent`. A label the repository lacks is added to it first. -3. It starts a child `thirdshift` on that issue from the same Launch directory, as a Merge run into the Run's Base branch, whatever the Run's own goal. Its progress lines are relayed with a `#: ` prefix, after `starting Base fix # into : ` and `waiting on Base fix #`. The Base fix sends no Run notification, treats every red check as its own to fix rather than as an Inherited failure, and never starts a Base fix of its own. +3. It starts a child `thirdshift` on that issue from the same Launch directory, as a Merge run into the Run's Base branch, whatever the Run's own goal. Its progress lines are relayed with a `#: ` prefix, after `starting Base fix # into : ` and `waiting on Base fix #`. The Base fix sends no Run notification, makes no [Claim](#the-claim) on its issue, treats every red check as its own to fix rather than as an Inherited failure, and never starts a Base fix of its own. 4. Once the Base fix has merged, and its Self-merge has closed its issue, the Run merges the Base branch in again and watches CI, and goes on as usual: ready for review, or merged in a Merge run. If the Base fix fails, the Run is a [Failed run](#failed-runs) with the cause `Base fix failed: `. If the checks are still Inherited failures once the Base fix has merged, there is no second one, and the cause is `CI red on , which also fails on at , even after Base fix merged; fix first`. The Run's last progress lines include `Base fix: merged`, or `not merged`, and its own [Run notification](#run-notifications) has the same `Base fix:` line. @@ -375,6 +389,8 @@ In a [Spec run](#spec-runs), a Ticket's Run says this on its own stderr, relayed A **Spec** is an issue with sub-issues, its **Tickets**. `thirdshift ` on a Spec is a **Spec run**: it works through the Tickets in the order their GitHub "blocked by" links allow, each Ticket's **Run** a **Merge run** into the **Spec branch**, then leaves one **Spec PR** from the Spec branch into the Base branch ready for review ([ADR-0006](docs/adr/0006-spec-runs-merge-tickets-into-a-spec-branch.md)). An issue with no sub-issues is an ordinary Run. +A Spec run makes the [Claim](#the-claim) on the Spec, as a Run does on its issue: the Spec is labelled `in-progress`, in place of `ready-for-agent`. Its Tickets' Runs change no label on their Tickets. + The Spec PR opens as a draft as soon as the first Ticket lands, titled from the Spec, with `Closes #` and a Tickets checklist: one line per Ticket, ticked once it is done, with its pull request, or saying it is running, failed, blocked or unready. thirdshift rewrites the checklist between its `` markers as each Ticket starts and ends, leaving the rest of the body as it is. Once every Ticket is done, the **Spec review** rewrites the body, and thirdshift puts the checklist back, appending it if the markers are gone, before marking the Spec PR ready. The Spec review session reviews the whole Spec branch against the Base branch and the Spec. Once it is marked ready, the Spec PR goes through the same step 6 as a Run's pull request, with the Spec as the issue and the Spec branch as the Issue branch: the Base branch is merged in (never rebased), CI watched, and a conflict or red CI handed to a Repair, within the same budgets. Inherited failures are told apart the same way everywhere: the Spec PR's against the Base branch, and a Ticket's against the Spec branch, its Base branch, with the cause shown on the Ticket's line of the checklist. @@ -440,9 +456,9 @@ An Architect run: 4. Removes the worktree and the temporary plugin directory once the session ends, whatever the outcome. 5. 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`). 6. Marks the plan ready, in one request: `needs-triage` is swapped for `ready-for-agent`, the plan is labelled `architect-plan`, the mark a later Architect run finds an open **Architect plan** by, and its other labels are kept. thirdshift adds that label, never the agent, and first creates it if the repository has none. A Spec's Tickets are left as the review labelled them, without `architect-plan`. -7. Dispatches the plan, unless given `--plan-only`, exactly as `thirdshift ` would from the same clone on the Architect run's Base branch: a [Spec run](#spec-runs) when the plan has sub-issues, a Run otherwise. Its Base branch is the Architect run's, so with `base ` it is ``, not the branch checked out. `merge` and `no-merge` apply to the Spec PR or the Run's pull request, `parallel ` to the Spec run, and `base-fix` and `no-base-fix` to whether that run may start a [Base fix](#base-fix), as if given to that command, and the [User config](#user-config) sets what they leave unsaid: `merge.always`, `spec.parallel`, `base.fix`, `launch.pull` and `logs.dir`. The Architecture review itself never watches CI, so a Base fix can only happen in the dispatched run. The one difference is that it sends no [Run notification](#run-notifications) of its own, whatever `email.always` says: the Architect run sends [the one](#one-run-notification). +7. Dispatches the plan, unless given `--plan-only`, exactly as `thirdshift ` would from the same clone on the Architect run's Base branch: a [Spec run](#spec-runs) when the plan has sub-issues, a Run otherwise. Its Base branch is the Architect run's, so with `base ` it is ``, not the branch checked out. `merge` and `no-merge` apply to the Spec PR or the Run's pull request, `parallel ` to the Spec run, and `base-fix` and `no-base-fix` to whether that run may start a [Base fix](#base-fix), as if given to that command, and the [User config](#user-config) sets what they leave unsaid: `merge.always`, `spec.parallel`, `base.fix`, `launch.pull` and `logs.dir`. The Architecture review itself never watches CI, so a Base fix can only happen in the dispatched run. It makes the [Claim](#the-claim) as that command would too: once its checks pass, the plan's `ready-for-agent` is swapped for `in-progress`, and the plan keeps `architect-plan`. The one difference is that it sends no [Run notification](#run-notifications) of its own, whatever `email.always` says: the Architect run sends [the one](#one-run-notification). -The dispatched run's ending is the Architect run's: its exit code, its pull request's URL alone on stdout, and its last line on stderr, `PR is ready for review` or `PR is merged`. If it fails, the Architect run fails as that Failed run or Failed spec run does, with the cause and the session log on stderr and the pull request's URL on stdout if it left one. The plan stays open, `ready-for-agent` and `architect-plan`, for `thirdshift ` to take up again: no later Architect run [retries it](#one-architect-plan-at-a-time). `parallel ` on a plan that is a single Ticket fails the same way as it does for `thirdshift ` on an issue that isn't a Spec: `parallel is only for a Spec, and # has no sub-issues`, exit `1`, before any implementing, with the plan left ready to run without it. +The dispatched run's ending is the Architect run's: its exit code, its pull request's URL alone on stdout, and its last line on stderr, `PR is ready for review` or `PR is merged`. If it fails, the Architect run fails as that Failed run or Failed spec run does, with the cause and the session log on stderr and the pull request's URL on stdout if it left one. The plan stays open, `in-progress` and `architect-plan`, for `thirdshift ` to take up again: no later Architect run [retries it](#one-architect-plan-at-a-time). `parallel ` on a plan that is a single Ticket fails the same way as it does for `thirdshift ` on an issue that isn't a Spec: `parallel is only for a Spec, and # has no sub-issues`, exit `1`, before any implementing and before the Claim, with the plan left `ready-for-agent`, to run without it. With `--plan-only`, it instead exits `0` with the plan's URL alone on stdout, and `plan is ready for an agent` as stderr's last line. Read or edit the plan, then run it with `thirdshift `. The plan is labelled `architect-plan` here too, so until it is finished or closed the next Architect run is [skipped](#one-architect-plan-at-a-time). diff --git a/src/architect.rs b/src/architect.rs index f3af98770..91417a1a4 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -384,15 +384,7 @@ fn mark_plan_ready(plan: &IssueUrl, origin: &str, started: DateTime) -> Res &plan.repo_slug(), &[(ARCHITECT_PLAN_LABEL, ARCHITECT_PLAN_DESCRIPTION)], )?; - let added = [READY_FOR_AGENT, ARCHITECT_PLAN_LABEL]; - // GitHub's label names are case-insensitive. - let mut labels: Vec<&str> = kept - .iter() - .map(String::as_str) - .filter(|kept| !added.iter().any(|added| added.eq_ignore_ascii_case(kept))) - .collect(); - labels.extend(added); - github::set_labels(plan, &labels) + github::set_labels_adding(plan, &kept, &[READY_FOR_AGENT, ARCHITECT_PLAN_LABEL]) } /// The labels `plan` keeps once it is marked ready: all it has but diff --git a/src/claim.rs b/src/claim.rs new file mode 100644 index 000000000..e1c537cc0 --- /dev/null +++ b/src/claim.rs @@ -0,0 +1,55 @@ +//! The Claim: the mark that the factory has taken an issue, the label +//! `in-progress` in place of `ready-for-agent`, so the issue list shows what a +//! Run or a Spec run is working on. + +use anyhow::{Context, Result}; + +use crate::github; +use crate::issue::IssueUrl; +use crate::progress; +use crate::spec_run::READY_FOR_AGENT; + +/// The label of a Claimed issue. +const IN_PROGRESS: &str = "in-progress"; + +/// The description the `in-progress` label is added to the repository with +/// if the repository lacks it. +const IN_PROGRESS_DESCRIPTION: &str = "A Claim: thirdshift has taken this issue"; + +/// Make the Claim on `issue`: label it `in-progress`, in place of +/// `ready-for-agent` if it has that, in one request that keeps its other +/// labels, having added `in-progress` to the repository if it lacks it. An +/// issue already Claimed, `in-progress` and not `ready-for-agent`, is left as +/// it is, with no request made. A failure names the Claim as what could not +/// be made. +pub fn make(issue: &IssueUrl) -> Result<()> { + label_in_progress(issue) + .with_context(|| format!("could not make the Claim on #{}", issue.number)) +} + +/// [`make`], its failure as `gh` gave it. +fn label_in_progress(issue: &IssueUrl) -> Result<()> { + let mut labels = github::issue_labels(issue)?; + // GitHub's label names are case-insensitive. + let has = |label: &str| labels.iter().any(|name| name.eq_ignore_ascii_case(label)); + let (claimed, ready) = (has(IN_PROGRESS), has(READY_FOR_AGENT)); + if claimed && !ready { + return Ok(()); + } + if ready { + progress::step(format_args!( + "labelling #{} {IN_PROGRESS}, in place of {READY_FOR_AGENT}", + issue.number + )); + } else { + progress::step(format_args!("labelling #{} {IN_PROGRESS}", issue.number)); + } + if !claimed { + github::ensure_labels( + &issue.repo_slug(), + &[(IN_PROGRESS, IN_PROGRESS_DESCRIPTION)], + )?; + } + labels.retain(|name| !name.eq_ignore_ascii_case(READY_FOR_AGENT)); + github::set_labels_adding(issue, &labels, &[IN_PROGRESS]) +} diff --git a/src/github.rs b/src/github.rs index 5436d3f56..70a7279eb 100644 --- a/src/github.rs +++ b/src/github.rs @@ -58,22 +58,32 @@ pub fn issue(issue: &IssueUrl) -> Result { .context("gh output has no createdAt")?; Ok(Issue { is_open: state_is_open(&json)?, - labels: json["labels"] - .as_array() - .context("gh output has no labels")? - .iter() - .filter_map(|label| label["name"].as_str().map(String::from)) - .collect(), + labels: label_names(&json)?, created: DateTime::parse_from_rfc3339(created) .with_context(|| format!("gh output has an unreadable createdAt {created}"))? .to_utc(), }) } +/// The names of the labels of the issue `json` describes, with its `labels`. +fn label_names(json: &Value) -> Result> { + Ok(json["labels"] + .as_array() + .context("gh output has no labels")? + .iter() + .filter_map(|label| label["name"].as_str().map(String::from)) + .collect()) +} + +/// The labels of `issue`. +pub fn issue_labels(issue: &IssueUrl) -> Result> { + label_names(&issue_view(issue, "labels")?) +} + /// Set `issue`'s labels to exactly `labels`, in one request, so a swap of /// one label for another can't stop halfway. Through the REST API: `gh issue /// edit` fails on the GitHub Projects (classic) sunset in older `gh`. -pub fn set_labels(issue: &IssueUrl, labels: &[&str]) -> Result<()> { +fn set_labels(issue: &IssueUrl, labels: &[&str]) -> Result<()> { let path = format!("repos/{}/issues/{}/labels", issue.repo_slug(), issue.number); let fields: Vec = labels .iter() @@ -86,6 +96,20 @@ pub fn set_labels(issue: &IssueUrl, labels: &[&str]) -> Result<()> { gh(&args) } +/// Set `issue`'s labels to `kept` and then each of `added`, in one request, +/// as [`set_labels`] does. One of `kept` that is also one of `added`, +/// whatever its case, as GitHub's label names are case-insensitive, is there +/// once, as `added` spells it. +pub fn set_labels_adding(issue: &IssueUrl, kept: &[String], added: &[&str]) -> Result<()> { + let mut labels: Vec<&str> = kept + .iter() + .map(String::as_str) + .filter(|kept| !added.iter().any(|added| added.eq_ignore_ascii_case(kept))) + .collect(); + labels.extend(added); + set_labels(issue, &labels) +} + /// A Spec's sub-issue, as a Spec run reads it. pub struct Ticket { pub number: u64, diff --git a/src/main.rs b/src/main.rs index 7c4c5b8c9..d887c5efe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,6 +4,7 @@ mod base_fix; mod branch; mod child_run; mod ci; +mod claim; mod config; mod email; mod failed_run; @@ -75,6 +76,12 @@ fails, naming the Base fix issue. A Run that finds an open base-fix issue for th branch and checks waits for that one to close instead of starting another, and a Spec run's Tickets that meet the same Inherited failure share one Base fix. +A Run or a Spec run makes the Claim on its issue once its checks pass, before any work: it +labels the issue in-progress, in place of ready-for-agent if it has that, keeping its other +labels and creating the label if the repository lacks it. A Ticket's Run in a Spec run and a +Base fix make none, and an issue already in-progress is left as it is. A Run whose Claim +can't be made stops there, naming the cause. + On a Spec, an issue with sub-issues, the Run is a Spec run: it takes every Ticket (sub-issue) it can reach, in the order their \"blocked by\" links allow, each merged into the Spec branch. Its Spec PR opens as a draft, with a Tickets checklist, once the first Ticket lands, and is marked ready @@ -104,8 +111,8 @@ a single Ticket. thirdshift then checks that the plan is open, new and not label ready-for-human, needs-info or wontfix, swaps its needs-triage label for ready-for-agent, labels it architect-plan, creating the label if the repository lacks it, and dispatches it as thirdshift would: a Spec run on a Spec, a Run on a single Ticket. A Spec's -Tickets are never labelled architect-plan. The Architect run ends as that run does, with -its exit code and its PR's URL. +Tickets are never labelled architect-plan. That run makes the Claim on the plan, which keeps +architect-plan. The Architect run ends as that run does, with its exit code and its PR's URL. merge, --no-merge, base-fix, --no-base-fix and parallel apply to that run, as do the User config's defaults; parallel fails it if the plan is a single Ticket. The review itself watches no CI, so only that run can start a Base fix. With --plan-only, the diff --git a/src/run.rs b/src/run.rs index 534e155d9..06f1a7057 100644 --- a/src/run.rs +++ b/src/run.rs @@ -9,6 +9,7 @@ use crate::base_fix::{Advice, BaseFix, BaseFixAsk}; use crate::branch::{self, Selection}; use crate::child_run::Kind; use crate::ci::{self, Ci}; +use crate::claim; use crate::failed_run::{self, FailedRun, PolicyRefusal}; use crate::git::Git; use crate::github::{self, Mergeable, PullRequest}; @@ -87,6 +88,13 @@ impl<'a> StartedBy<'a> { } } + /// Whether the Run, or the Spec run, makes the Claim on its issue: a + /// child Run makes none, so that only the issue the Day shift would look + /// at carries one. + fn makes_claim(self) -> bool { + self.child().is_none() + } + /// The Base branch the Run was given, if what started it gave one. fn given_base(self) -> Option<&'a str> { match self { @@ -144,6 +152,10 @@ pub fn run_to_end( /// with no sub-issues fails before any work, as does a Spec whose Tickets are /// all closed with no Spec branch to continue. /// +/// Once those checks pass, and before the worktree is created, the Run, or +/// the Spec run, makes the Claim on `issue`, unless it is a child Run. A Claim +/// that can't be made fails it there, before any work. +/// /// `base_fix` is the one Base fix the Run, or a Spec run for its Spec PR, may /// start, or wait on, when its only red checks are Inherited failures. fn run( @@ -190,6 +202,9 @@ fn run( if interrupt::requested() { return Err(anyhow!("interrupted").into()); } + if started_by.makes_claim() { + claim::make(issue)?; + } let worktree = match &selection { Selection::Fresh { .. } => Worktree::create_fresh(&launch, &issue.repo, &branch, &base)?, Selection::Continuation { .. } => { diff --git a/src/spec_run.rs b/src/spec_run.rs index 4479bed34..8e12d7ffe 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -32,8 +32,8 @@ use crate::worktree::Worktree; pub const NEEDS_TRIAGE: &str = "needs-triage"; /// The triage label of an issue an agent can take on: what thirdshift swaps -/// a plan's `needs-triage` for once the plan passes its checks, and what a -/// Base fix issue is opened with. +/// a plan's `needs-triage` for once the plan passes its checks, what a Base +/// fix issue is opened with, and what a Claim takes off. pub const READY_FOR_AGENT: &str = "ready-for-agent"; /// The triage labels that make an open Ticket an Unready Ticket. diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 709de2a7d..c3e2163b1 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -32,6 +32,10 @@ const PR_URL: &str = "https://github.com/acme/widgets/pull/1"; /// The label thirdshift marks an Architect plan with. const ARCHITECT_PLAN: &str = "architect-plan"; +/// The label of a Claimed issue, which the run a plan is dispatched as +/// swaps the plan's `ready-for-agent` for. +const IN_PROGRESS: &str = "in-progress"; + const NO_FINAL_LINE: &str = "the Architecture review ended without the final line its prompt asks for"; @@ -46,9 +50,7 @@ printf 'Published the plan.\n\nArchitecture review plan: %s\n' "$url" > "$FAKE_C /// issues with, as `gh issue create` refuses a label the repository lacks. fn scenario() -> Scenario { let scenario = Scenario::new(); - let mut gh = scenario.gh_state(); - gh["repo_labels"] = serde_json::json!(["needs-triage", "ready-for-agent", "architecture"]); - scenario.write_gh_state(&gh); + scenario.repo_has_labels(&["needs-triage", "ready-for-agent", "architecture"]); scenario } @@ -250,23 +252,17 @@ printf 'Architecture review plan: %s\n' "$spec" > "$FAKE_CLAUDE_FINAL_MESSAGE" assert_eq!(scenario.issue_labels(9), ["ready-for-agent"]); } -/// The repository's labels on the fake GitHub. -fn repo_labels(scenario: &Scenario) -> Vec { - let labels = scenario.gh_state()["repo_labels"].clone(); - serde_json::from_value(labels).unwrap() -} - #[test] fn the_architect_plan_label_is_created_when_the_repository_lacks_it() { let scenario = scenario(); scenario.agent_does(AGENT_PUBLISHES_A_TICKET); - assert!(!repo_labels(&scenario).contains(&ARCHITECT_PLAN.to_string())); + assert!(!scenario.repo_labels().contains(&ARCHITECT_PLAN.to_string())); let result = scenario.run(&["architect", "--plan-only"]); assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); assert_eq!( - repo_labels(&scenario), + scenario.repo_labels(), [ "needs-triage", "ready-for-agent", @@ -282,15 +278,13 @@ fn the_architect_plan_label_is_created_when_the_repository_lacks_it() { #[test] fn a_repository_that_has_the_architect_plan_label_keeps_it_as_it_is() { let scenario = scenario(); - let mut gh = scenario.gh_state(); - gh["repo_labels"] = serde_json::json!(["needs-triage", "Architect-Plan"]); - scenario.write_gh_state(&gh); + scenario.repo_has_labels(&["needs-triage", "Architect-Plan"]); scenario.agent_does(AGENT_PUBLISHES_A_TICKET); let result = scenario.run(&["architect", "--plan-only"]); assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert_eq!(repo_labels(&scenario), ["needs-triage", "Architect-Plan"]); + assert_eq!(scenario.repo_labels(), ["needs-triage", "Architect-Plan"]); assert!(scenario.gh_calls_of("label", "create").is_empty()); assert_eq!( scenario.issue_labels(8), @@ -698,10 +692,7 @@ fn a_single_ticket_plan_starts_a_run_on_it_and_ends_as_that_run_does() { let pr = pr_from(&scenario, "issue-8"); assert_ended_with_pr(&result, &pr, "ready for review"); assert_eq!(pr["url"], PR_URL); - assert_eq!( - scenario.issue_labels(8), - ["ready-for-agent", ARCHITECT_PLAN] - ); + assert_eq!(scenario.issue_labels(8), [ARCHITECT_PLAN, IN_PROGRESS]); let calls = scenario.claude_calls(); assert_eq!(calls.len(), 2, "sessions: {calls:?}"); let prompt = calls[1]["prompt"].as_str().unwrap(); @@ -730,10 +721,7 @@ fn a_plan_with_tickets_starts_a_spec_run_on_it_and_ends_as_that_spec_run_does() assert_eq!(spec_pr["base"], "main"); assert_eq!(spec_pr["state"], "OPEN"); assert_eq!(spec_pr["isDraft"], false); - assert_eq!( - scenario.issue_labels(8), - ["ready-for-agent", ARCHITECT_PLAN] - ); + assert_eq!(scenario.issue_labels(8), [ARCHITECT_PLAN, IN_PROGRESS]); let dispatch = format!("dispatching the plan {PLAN_URL}, as thirdshift {PLAN_URL} would\n"); let dispatched = result.stderr.find(&dispatch); let started = result.stderr.find("thirdshift: starting #9\n"); @@ -1040,10 +1028,7 @@ fn a_dispatched_run_that_fails_fails_the_architect_run_as_a_failed_run_does() { "stderr: {}", result.stderr ); - assert_eq!( - scenario.issue_labels(8), - ["ready-for-agent", ARCHITECT_PLAN] - ); + assert_eq!(scenario.issue_labels(8), [ARCHITECT_PLAN, IN_PROGRESS]); } #[test] diff --git a/tests/base_fix.rs b/tests/base_fix.rs index 1276718b3..404b3f326 100644 --- a/tests/base_fix.rs +++ b/tests/base_fix.rs @@ -589,10 +589,26 @@ fn without_base_fix_an_inherited_failure_fails_the_run_and_writes_no_issue() { result.stderr ); assert!(scenario.gh_calls_of("issue", "create").is_empty()); - assert!(scenario.gh_calls_of("label", "list").is_empty()); + // Only the Run's own Claim adds a label to the repository. + assert_eq!(scenario.repo_labels(), ["in-progress"]); assert_eq!(scenario.gh_state()["issues"].as_object().unwrap().len(), 1); } +#[test] +fn a_base_fix_changes_no_label_on_its_issue_while_the_run_claims_its_own() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); + scenario.agent_does_for(8, &format!("{BASE_FIX_OPENS_PR}{GREEN_ON_HEAD}")); + + let result = scenario.run(&[&scenario.issue_url(7), "base-fix"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.issue_labels(8), ["base-fix", "ready-for-agent"]); + assert_eq!(scenario.issue_labels(7), ["in-progress"]); + assert!(!result.stderr.contains("labelling #8"), "{}", result.stderr); +} + /// The cause of a Run that failed on `test`, an Inherited failure from /// `main` at `base_commit`, with no Base fix taken. fn inherited_failure(base_commit: &str) -> String { @@ -750,9 +766,7 @@ fn a_base_branch_check_with_no_url_is_named_alone() { #[test] fn a_label_the_repository_lacks_is_added_and_one_it_has_is_left_alone() { let scenario = Scenario::new(); - let mut gh = scenario.gh_state(); - gh["repo_labels"] = serde_json::json!(["bug", "Ready-For-Agent"]); - scenario.write_gh_state(&gh); + scenario.repo_has_labels(&["bug", "Ready-For-Agent", "in-progress"]); scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); scenario.agent_does_for(8, &format!("{BASE_FIX_OPENS_PR}{GREEN_ON_HEAD}")); @@ -763,8 +777,8 @@ fn a_label_the_repository_lacks_is_added_and_one_it_has_is_left_alone() { assert_eq!(created.len(), 1, "{created:?}"); assert_eq!(created[0][2], "base-fix"); assert_eq!( - scenario.gh_state()["repo_labels"], - serde_json::json!(["bug", "Ready-For-Agent", "base-fix"]) + scenario.repo_labels(), + ["bug", "Ready-For-Agent", "in-progress", "base-fix"] ); } diff --git a/tests/command_surface.rs b/tests/command_surface.rs index b104a1c35..b2bf3811c 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -203,6 +203,23 @@ fn help_documents_base_for_an_architect_run() { } } +#[test] +fn help_documents_the_claim() { + let scenario = Scenario::new(); + + let help = unwrapped_help(&scenario); + + for mention in [ + "A Run or a Spec run makes the Claim on its issue once its checks pass, before any work: it labels the issue in-progress, in place of ready-for-agent if it has that", + "creating the label if the repository lacks it", + "A Ticket's Run in a Spec run and a Base fix make none", + "A Run whose Claim can't be made stops there", + "That run makes the Claim on the plan, which keeps architect-plan", + ] { + assert!(help.contains(mention), "help lacks {mention:?}: {help}"); + } +} + #[test] fn help_points_to_running_an_architect_run_on_a_schedule_without_the_recipe() { let scenario = Scenario::new(); diff --git a/tests/continuation.rs b/tests/continuation.rs index 1788f4f24..bb46640a4 100644 --- a/tests/continuation.rs +++ b/tests/continuation.rs @@ -283,13 +283,15 @@ fn selection_asks_github_once_for_the_pr_history() { scenario.run(&[&scenario.issue_url(7)]); // Pre-flight's `issue view` comes first, then the `api graphql` that finds - // no sub-issues; everything after it and before the post-session - // `pr view` is selection. The agent here makes no gh calls of its own. + // no sub-issues; what comes after it is selection, up to the Claim, which + // starts at the next `issue view`. let subcommands: Vec = scenario .gh_calls() .iter() .map(|argv| argv[..2].join(" ")) - .take_while(|subcommand| subcommand != "pr view") + .enumerate() + .take_while(|(at, subcommand)| *at == 0 || subcommand != "issue view") + .map(|(_, subcommand)| subcommand) .collect(); assert_eq!(subcommands, vec!["issue view", "api graphql", "pr list"]); assert!( @@ -319,3 +321,46 @@ fn an_open_pr_supplies_the_base_branch_even_on_a_detached_head() { scenario.first_prompt() ); } + +#[test] +fn a_continuation_on_an_issue_already_claimed_leaves_its_labels_as_they_are() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + scenario.issue_labelled(7, &["in-progress", "bug"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.issue_labels(7), ["in-progress", "bug"]); + assert!(scenario.gh_calls_of("api", "--method").is_empty()); + assert!(scenario.gh_calls_of("label", "create").is_empty()); + assert!(!result.stderr.contains("labelling #7"), "{}", result.stderr); +} + +#[test] +fn a_continuation_on_an_issue_not_yet_claimed_claims_it() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + scenario.issue_labelled(7, &["ready-for-agent", "bug"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.issue_labels(7), ["bug", "in-progress"]); +} + +#[test] +fn a_claimed_issue_marked_ready_again_loses_ready_for_agent_and_stays_claimed() { + let scenario = Scenario::new(); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + scenario.issue_labelled(7, &["in-progress", "bug", "ready-for-agent"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.issue_labels(7), ["bug", "in-progress"]); + assert!(scenario.gh_calls_of("label", "list").is_empty()); +} diff --git a/tests/fresh_run.rs b/tests/fresh_run.rs index 52fcc9ab5..70d8c5e1a 100644 --- a/tests/fresh_run.rs +++ b/tests/fresh_run.rs @@ -253,3 +253,128 @@ fn another_issues_branch_and_pr_do_not_count_as_this_issues() { ]) ); } + +#[test] +fn a_fresh_run_claims_the_issue_in_place_of_ready_for_agent_and_keeps_its_other_labels() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["bug", "ready-for-agent", "architecture"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!( + scenario.issue_labels(7), + ["bug", "architecture", "in-progress"] + ); +} + +#[test] +fn a_run_on_an_issue_with_no_ready_for_agent_label_still_claims_it() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["bug"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.issue_labels(7), ["bug", "in-progress"]); + assert!( + result + .stderr + .contains("thirdshift: labelling #7 in-progress\n"), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn the_claim_is_made_in_one_request_before_the_worktree_is_created() { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let claimed = result + .stderr + .find("thirdshift: labelling #7 in-progress, in place of ready-for-agent\n"); + let worktree = result.stderr.find("thirdshift: creating worktree "); + assert!( + claimed.is_some() && claimed < worktree, + "stderr: {}", + result.stderr + ); + let requests = scenario.gh_calls_of("api", "--method"); + assert_eq!(requests.len(), 1, "{requests:?}"); + assert_eq!( + requests[0][2..], + [ + "PUT", + "repos/acme/widgets/issues/7/labels", + "--silent", + "-f", + "labels[]=in-progress" + ] + ); +} + +#[test] +fn the_in_progress_label_is_created_when_the_repository_lacks_it() { + let scenario = Scenario::new(); + scenario.repo_has_labels(&["bug", "ready-for-agent"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!( + scenario.repo_labels(), + ["bug", "ready-for-agent", "in-progress"] + ); + let created = scenario.gh_calls_of("label", "create"); + assert_eq!(created.len(), 1, "{created:?}"); + assert_eq!(created[0][2], "in-progress"); +} + +#[test] +fn a_repository_that_has_the_in_progress_label_keeps_it_as_it_is() { + let scenario = Scenario::new(); + scenario.repo_has_labels(&["In-Progress"]); + scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.repo_labels(), ["In-Progress"]); + assert!(scenario.gh_calls_of("label", "create").is_empty()); + assert_eq!(scenario.issue_labels(7), ["in-progress"]); +} + +#[test] +fn a_run_whose_claim_cannot_be_made_stops_before_any_work_naming_the_cause() { + for (failing, cause) in [ + ("label list", "gh label list failed: HTTP 502"), + ("label create", "gh label create in-progress "), + ( + "api --method", + "gh api --method PUT repos/acme/widgets/issues/7/labels --silent \ + -f labels[]=in-progress failed: HTTP 502", + ), + ] { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.gh_fails(failing); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + scenario.assert_rejected_before_any_work( + &result, + &format!("thirdshift: could not make the Claim on #7: {cause}"), + ); + assert_eq!(result.code, Some(1), "{failing}"); + assert_eq!(scenario.issue_labels(7), ["ready-for-agent"], "{failing}"); + assert!(scenario.origin_log("issue-7").is_none(), "{failing}"); + } +} diff --git a/tests/preflight.rs b/tests/preflight.rs index 506a52c16..1a06fe2c6 100644 --- a/tests/preflight.rs +++ b/tests/preflight.rs @@ -179,3 +179,68 @@ fn a_git_identity_set_only_in_the_launch_repository_is_enough() { assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); } + +/// Assert that the Run `args` start on #`issue`, labelled `bug` and +/// `ready-for-agent`, is stopped with `message` before any work, with the +/// issue's labels and the repository's as they were. +fn assert_stopped_with_no_label_changed( + scenario: &Scenario, + issue: u32, + args: &[&str], + message: &str, +) { + scenario.issue_labelled(issue, &["bug", "ready-for-agent"]); + + let result = scenario.run(args); + + scenario.assert_rejected_before_any_work(&result, message); + assert_eq!( + scenario.issue_labels(issue), + ["bug", "ready-for-agent"], + "{message}" + ); + assert!(scenario.repo_labels().is_empty(), "{message}"); +} + +#[test] +fn a_run_stopped_by_a_preflight_check_changes_no_label() { + let scenario = Scenario::new(); + scenario.issue_is(7, "CLOSED"); + assert_stopped_with_no_label_changed( + &scenario, + 7, + &[&scenario.issue_url(7)], + "issue #7 is closed", + ); + + let scenario = Scenario::new(); + scenario.commit_locally("unpushed.txt", "unpushed\n", "Unpushed work"); + assert_stopped_with_no_label_changed( + &scenario, + 7, + &[&scenario.issue_url(7)], + "local main is 1 commit(s) ahead of origin/main", + ); + + let scenario = Scenario::new(); + assert_stopped_with_no_label_changed( + &scenario, + 7, + &[&scenario.issue_url(7), "parallel", "2"], + "parallel is only for a Spec, and #7 has no sub-issues", + ); +} + +#[test] +fn a_spec_run_stopped_by_a_preflight_check_changes_no_label() { + let scenario = Scenario::new(); + scenario.spec_has_tickets(20, &[(21, &[])]); + scenario.issue_is(21, "CLOSED"); + + assert_stopped_with_no_label_changed( + &scenario, + 20, + &[&scenario.issue_url(20)], + "every Ticket is closed and there is no Spec branch; nothing to do", + ); +} diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 4832b4a19..29438668e 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -151,6 +151,28 @@ fn a_linear_spec_lands_each_ticket_in_order_then_opens_a_ready_spec_pr() { scenario.assert_cleaned_up("issue-20"); } +#[test] +fn a_spec_run_claims_the_spec_and_its_tickets_runs_change_no_label() { + let scenario = linear_spec(); + scenario.issue_labelled(SPEC, &["ready-for-agent", "enhancement"]); + for ticket in [21, 22] { + scenario.issue_labelled(ticket, &["ready-for-agent"]); + } + + let result = scenario.run(&[&spec_url(&scenario)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.issue_labels(SPEC), ["enhancement", "in-progress"]); + for ticket in [21, 22] { + assert_eq!(scenario.issue_labels(ticket), ["ready-for-agent"]); + } + assert_contains( + &result.stderr, + "thirdshift: labelling #20 in-progress, in place of ready-for-agent\n", + ); + assert_eq!(result.stderr.matches(" in-progress").count(), 1); +} + #[test] fn each_ticket_branches_off_the_spec_branch_and_its_prompts_name_it_as_the_base() { let scenario = linear_spec(); diff --git a/tests/support/mod.rs b/tests/support/mod.rs index f901aeeaa..9ebe774ba 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -624,6 +624,22 @@ test -f {root}/{COPY_REPLACED} .collect() } + /// Give the repository the labels `labels` on the fake GitHub. + pub fn repo_has_labels(&self, labels: &[&str]) { + let mut gh = self.gh_state(); + gh["repo_labels"] = json!(labels); + self.write_gh_state(&gh); + } + + /// The repository's labels on the fake GitHub. + pub fn repo_labels(&self) -> Vec { + let labels = &self.gh_state()["repo_labels"]; + let labels = labels.as_array().into_iter().flatten(); + labels + .map(|label| label.as_str().unwrap().to_string()) + .collect() + } + /// Give issue `number` the title `title` on the fake GitHub. pub fn issue_titled(&self, number: u32, title: &str) { let mut gh = self.gh_state(); @@ -641,13 +657,19 @@ test -f {root}/{COPY_REPLACED} /// Make every `gh api user` call fail. pub fn github_profile_fails(&self) { + self.gh_fails("api user"); + } + + /// Make every `gh ` fail, `call` being the command's first two + /// arguments, e.g. `label list`. + pub fn gh_fails(&self, call: &str) { let mut gh = self.gh_state(); let failing = gh.as_object_mut().unwrap().entry("failing"); failing .or_insert(json!([])) .as_array_mut() .unwrap() - .push(json!("api user")); + .push(json!(call)); self.write_gh_state(&gh); }