From 944a97661e8b08f750e7ffd70b7fbae286bc88f2 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 26 Oct 2026 22:11:40 -0400 Subject: [PATCH 1/3] Publish an Issue-branch Process through Delivery and remove the Claim (#1388) A Process can name one issue-branch worktree and a delivery Step. The run creates the Issue branch, holds the Claim, and leaves a ready-for-review pull request. A missing issue or a missing delivery is refused before any session. --- README.md | 2 +- docs/guide.md | 114 +++++- site/architecture/index.html | 2 +- site/docs/index.html | 106 ++++- site/how-it-works/index.html | 2 +- site/index.html | 2 +- src/args.rs | 13 +- src/claim.rs | 46 ++- src/delivery.rs | 107 +++++ src/main.rs | 4 +- src/processes.rs | 758 +++++++++++++++++++++++++++++++---- src/pull_request.rs | 21 + tests/process.rs | 563 +++++++++++++++++++++++++- 13 files changed, 1599 insertions(+), 141 deletions(-) diff --git a/README.md b/README.md index 25a09e5a..cf8a2124 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,7 @@ Use `thirdshift help` for the command reference, or read [Ticket run behavior](d | A security audit | `thirdshift secure` | A [Security run](docs/guide.md#security-runs) records findings privately; fixing a reproduced finding is optional. | | Merged factory work | `thirdshift score` | A [Scoring run](docs/guide.md#scoring-runs) samples and grades it, and proposes Bug origins; `thirdshift scores --json` lists samples, five criterion Scores, calibration and proposals by week; `thirdshift scores --mine` records yours before revealing the model's. | | One skill | `thirdshift skill [issue ]` | A [standalone skill](docs/guide.md#standalone-skills) runs headlessly and prints its final message. | -| A Custom process | `thirdshift process [issue ]` | An experimental [Custom process](docs/guide.md#custom-processes) runs skill or command Steps from `~/.thirdshift/processes/.toml`, in named throwaway worktrees when the file declares them. An optional Step can fail without stopping the process. `issue ` gives every session Step that issue. With no issue it is a Pass and skips while another Pass on the repository is running. Status, the Console and the Activity log list it as Process ``. A failed one is in Needs you, and it sends a Run notification under the same email rules as a Ticket run. A skipped Process sends none. | +| A Custom process | `thirdshift process [issue ]` | An experimental [Custom process](docs/guide.md#custom-processes) runs skill or command Steps from `~/.thirdshift/processes/.toml`, in named throwaway worktrees when the file declares them. An optional Step can fail without stopping the process. `issue ` gives every session Step that issue. One Issue-branch worktree plus a `delivery` Step publishes a ready-for-review pull request from that Issue branch and then removes the Claim. With no issue, and no Issue-branch worktree, it is a Pass and skips while another Pass on the repository is running. Status, the Console and the Activity log list it as Process ``. A failed one is in Needs you, and it sends a Run notification under the same email rules as a Ticket run. A skipped Process sends none. | Each Architect, Pickup or Security run makes one pass. Use cron or another operating system scheduler to repeat it. See [scheduling](docs/guide.md#on-a-schedule), [Architect runs every few minutes](docs/guide.md#architect-runs-every-few-minutes) and [Security runs on a schedule](docs/guide.md#security-runs-on-a-schedule-with-fixing-allowed). diff --git a/docs/guide.md b/docs/guide.md index 52a9a562..15caff69 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -506,7 +506,7 @@ thirdshift secure base # do that with as the Base branch, thirdshift skill [issue ] # run one skill headlessly and print its final message thirdshift process # list Custom processes (experimental) thirdshift process [issue ] # run a Custom process, optionally on an issue; email flags match a Ticket run (experimental) -thirdshift process check # validate one and print its plan (experimental) +thirdshift process check [issue ] # validate one and print its plan (experimental) thirdshift score # sample merged factory pull requests (see Scoring runs) thirdshift scores [] [--json] # show Scores and calibration thirdshift scores --mine [--json] # enter your five Scores @@ -843,8 +843,9 @@ Command record, Console entry or Session log is created for a standalone skill. ### Custom processes A Custom process runs Steps in order, and can run consecutive Steps together. -A Step is a skill session or a command. It is experimental. Write -`~/.thirdshift/processes/.toml`. The name is the file's stem. +A Step is a skill session, a command, or the catalogue Step `delivery`. It is +experimental. Write `~/.thirdshift/processes/.toml`. The name is the +file's stem. ```toml version = 1 @@ -909,6 +910,62 @@ skill = "thirdshift-to-spec" worktree = "judge" ``` +One worktree may instead be an Issue branch, and one Step may publish it: + +```toml +version = 1 +description = "Ship a fix" + +[worktrees.fix] +kind = "issue-branch" + +[[steps]] +name = "implement" +skill = "thirdshift-implement" +worktree = "fix" + +[[steps]] +name = "ship" +catalogue = "delivery" +worktree = "fix" +``` + +```sh +thirdshift process check ship issue 7 +thirdshift process ship issue 7 +``` + +A declared worktree's `kind` is `throwaway` or `issue-branch`. A throwaway +worktree is detached at the Base branch. An Issue-branch worktree is the +Issue branch a Ticket run would use for the issue the command names: +`issue-`, then `issue--branch-` for k ≥ 2. A Continuation reuses +the existing branch, and an open pull request's base is the Base branch. A +Process has at most one Issue-branch worktree. The Steps that name it run in +that checkout. Any other declared worktree stays throwaway. + +`catalogue = "delivery"` publishes that Issue branch. It is Delivery with no +opening session and no Security review: push, open a draft pull request when +the branch has none, write the Built-with line, mark the pull request ready +for review, and keep it mergeable with green or absent CI through the Repair +loop. It does not Self-merge, even when the User config asks Ticket runs to. +When thirdshift opens the pull request, its title is the issue's and its body +starts `Closes #`. Repairs run on the Command's Harness, Model and Effort. +A Process has at most one delivery Step. It names the Issue-branch worktree, +it is not optional, and it is not in a parallel group. It comes after the +last Step that uses that worktree. The only catalogue value is `delivery`. + +An Issue-branch worktree needs an issue. `thirdshift process ` and +`thirdshift process check ` refuse the file before any session, any +Claim and any branch, naming the `issue` rule and the file line. With an +issue, the run makes the Claim when it starts, labelling the issue +`in-progress` in place of `ready-for-agent`, and holds it until the end. A +pull request left ready for review removes the Claim and does not put +`ready-for-agent` back, so Pickup does not take an issue that already has an +open pull request. A Step that fails before Delivery keeps the Issue-branch +work as a Failed ticket run does, and the failure says where it stopped, what +was preserved and the command `thirdshift process issue `. The +Claim stays when that work reached origin, and is released when it did not. + `version` must be `1`. Each Step has a `name` and exactly one kind. `skill` is looked up the same way as `thirdshift skill`. First a Factory skill whose directory is exactly that name. Then the repository's committed @@ -922,9 +979,8 @@ skills are ([ADR-0012](adr/0012-factory-skills-linked-into-the-worktree-codex-unsandboxed.md)), under its own name, as a copy written out beside those skills, and kept out of git. A repository skill of the same name is not replaced. `command` is a command or script, -run with `sh -c` in the Step's worktree. It starts no session. A declared -worktree's `kind` must be `throwaway`: it is detached at the Base branch. When -the Process declares worktrees, every Step names one with `worktree`. +run with `sh -c` in the Step's worktree. It starts no session. When the +Process declares worktrees, every Step names one with `worktree`. Consecutive Steps with the same `parallel` name run at the same time. Every session or command Step edits the worktree it runs in, so Steps in one group cannot share a worktree. The same group name after a Step that is not in the @@ -935,7 +991,9 @@ failure stops the Process once the other Steps in its group have finished. ```sh thirdshift process thirdshift process check gather-review +thirdshift process check ship issue 7 thirdshift process gather-review +thirdshift process ship issue 7 ``` `thirdshift process` lists each Custom process and its description. `check` @@ -943,16 +1001,25 @@ validates the file and prints the plan: the description, the Command's Harness, Model and Effort, and each Step. A Process with no named worktrees prints one throwaway worktree at the Base branch. A Process that declares worktrees prints each one, the worktree each Step uses, and a `together:` line for each group -of Steps that run at the same time. A skill Step shows the skill and where it -was found. A command Step shows its command, and `check` does not run it. An -optional Step is marked `(optional)`. `check` starts no session. +of Steps that run at the same time. A throwaway worktree is described as +throwaway at the Base branch. An Issue-branch worktree is described as an +Issue branch. A skill Step shows the skill and where it was found. A command +Step shows its command, and `check` does not run it. A delivery Step shows +`delivery`. An optional Step is marked `(optional)`. `check` starts no +session. `check issue ` resolves the issue and still creates no +Issue branch and makes no Claim. Without an issue, an Issue-branch worktree +is the refusal above, not a plan. Running the Process starts the Steps in its throwaway worktrees, detached at the Base branch and cleaned up as a standalone skill's worktree is. Every throwaway worktree is removed at the end, including after a failure, unless uncommitted changes, commits absent from every remote-tracking branch, or -uncertain ownership keep it. The Base branch is the branch checked out in the -launch directory, at its commit on origin. The launch checkout stays untouched. +uncertain ownership keep it. The Issue-branch worktree is a real checkout of +the Issue branch, beside the launch repository, and is removed at the end +when its work is on origin, as a Ticket run's is. The Base branch is the +branch checked out in the launch directory, at its commit on origin, or the +open pull request's base in a Continuation. The launch checkout stays +untouched. Every session Step uses the Command's Harness, Model and Effort: the User config default, or the Harness default when that is unset. A command Step does @@ -988,14 +1055,19 @@ Trusted account, made since a few minutes before the Process started, fails that Step instead of starting the session. The Command log names the cause. `check` and running refuse a missing file, a file that is not valid TOML, a -missing or wrong `version`, an unknown key, a Step without exactly one of -`skill` and `command`, a command that is not a string or is empty, an -`optional` value that is not a boolean, a skill that cannot be found, an -unknown worktree, a Step that does not name a worktree when the Process -declares them, parallel Steps that share a worktree, a worktree whose kind is -not `throwaway`, and a reserved name (`check`, `list`, `ticket-run`, -`spec-run`, `architect-run`, `pickup-run`, `security-run`). The refusal names -the rule and the file line, and starts no session. +missing or wrong `version`, an unknown key, a Step without exactly one kind, +a command that is not a string or is empty, an `optional` value that is not a +boolean, a skill that cannot be found, an unknown worktree, a Step that does +not name a worktree when the Process declares them, parallel Steps that share +a worktree, a worktree kind other than `throwaway` or `issue-branch`, a +second Issue-branch worktree, a catalogue value other than `delivery`, a +delivery Step with no Issue-branch worktree or on a different worktree, a +second delivery Step, an optional delivery Step, a delivery Step in a +parallel group, an Issue-branch worktree with no delivery Step after the last +Step that uses it, an Issue-branch worktree with no issue, and a reserved +name (`check`, `list`, `ticket-run`, `spec-run`, `architect-run`, +`pickup-run`, `security-run`). The refusal names the rule and the file line, +and starts no session. The Command is recorded as kind `Process`, with the process name. Its Command log has one line per Step: the Step's name, duration, outcome and cause. A @@ -1014,7 +1086,9 @@ notification under the same `email`, `email-unless-merged` and `no-email` flags and the same User config rules as a Ticket run. A missing address or Resend key stops the Process before any Step. A skipped Process sends none. -A Custom process started with no issue is a [Pass](#one-at-a-time). While another Pass on the repository is still running, its Spec run or Ticket run included, `thirdshift process ` is skipped: it prints `another Pass is already running on /`, prints nothing on stdout, exits `0` and sends no Run notification. It creates no worktree and starts no Step. While it runs, a Pickup run on the same repository is skipped. It does not take an Architect run's Ready-issue or Architect-idea gates, so a Ready issue or an Architect idea waiting for triage does not skip it. The Command record's outcome is `skipped`, and its cause is that reason. The Activity log records `Process skipped: ` when that is not already the last Process line. With `activity.quiet_skips` set, a skipped Process prints nothing. `thirdshift process issue ` is not a Pass: this rule does not skip it, and it does not hold the lock. +A Custom process started with no issue is a [Pass](#one-at-a-time), except one +that declares an Issue-branch worktree: that is refused, as above, rather than +skipped. While another Pass on the repository is still running, its Spec run or Ticket run included, `thirdshift process ` is skipped: it prints `another Pass is already running on /`, prints nothing on stdout, exits `0` and sends no Run notification. It creates no worktree and starts no Step. While it runs, a Pickup run on the same repository is skipped. It does not take an Architect run's Ready-issue or Architect-idea gates, so a Ready issue or an Architect idea waiting for triage does not skip it. The Command record's outcome is `skipped`, and its cause is that reason. The Activity log records `Process skipped: ` when that is not already the last Process line. With `activity.quiet_skips` set, a skipped Process prints nothing. `thirdshift process issue ` is not a Pass: this rule does not skip it, and it does not hold the lock. ### User config diff --git a/site/architecture/index.html b/site/architecture/index.html index 5b96b267..93e44c6d 100644 --- a/site/architecture/index.html +++ b/site/architecture/index.html @@ -479,7 +479,7 @@

Architecture

Figure 5 · thirdshift pickup, architect, secure

The Passes: the factory finds its own work

Before selecting a Ready issue, Pickup files one ordinary bug Ticket when the Base branch's latest push CI failed more than 30 minutes ago and no CI-red Ticket is open. Concurrent hosts reserve filing on origin. CI read failures warn and leave Pickup running; this check does not hold Self-merges.

Once a Pickup run, an Architect run or a Security run knows its repository, it replaces that kind's last-fired time under the repository's log folder, before launch checks or the local Pass lock. Repeated skips update the time without adding Activity log lines. thirdshift status, thirdshift cronjobs and their Console views read the same per-repository times; no reader starts work. A Custom process started with no issue shares the lock and does not record a last-fired time.

-

A Pass takes no Issue URL, so your operating system's scheduler can start it; thirdshift never schedules itself. Only one Pass runs on a repository at a time, and a Pass with nothing to do is skipped, not failed. Architect and Security runs wait while a Ready issue is open, so work you shaped goes first. A Custom process started with no issue is a Pass for that one-at-a-time rule and does not wait on a Ready issue or an Architect idea. One started with an issue is not a Pass. Your scheduler can start Architect runs every few minutes and Security runs on a schedule, with fixing allowed.

+

A Pass takes no Issue URL, so your operating system's scheduler can start it; thirdshift never schedules itself. Only one Pass runs on a repository at a time, and a Pass with nothing to do is skipped, not failed. Architect and Security runs wait while a Ready issue is open, so work you shaped goes first. A Custom process started with no issue is a Pass for that one-at-a-time rule and does not wait on a Ready issue or an Architect idea, unless the file declares an Issue-branch worktree: that is refused before any session, rather than skipped. One started with an issue is not a Pass. Your scheduler can start Architect runs every few minutes and Security runs on a schedule, with fixing allowed.

The three PassesThe operating system’s scheduler starts three kinds of Pass, each taking no Issue URL. A Pickup run sweeps closed issues, respects the Claim limit and runs the lowest-numbered Ready issue by default. An Architect run reviews the architecture and either publishes a plan, which it builds with a Ticket run or Spec run, or files an idea for the Day shift to triage. A Security run audits the Base branch, reproduces findings, records each privately and, when fixing is allowed, files a fix issue and runs it. Its target code may run only inside an OS-enforced sandbox, as the Security audit skill requires. diff --git a/site/docs/index.html b/site/docs/index.html index 624ff4b5..0ae3530b 100644 --- a/site/docs/index.html +++ b/site/docs/index.html @@ -465,7 +465,7 @@ thirdshift skill <skill> [issue <issue>] # run one skill headlessly and print its final message thirdshift process # list Custom processes (experimental) thirdshift process <name> [issue <issue>] # run a Custom process, optionally on an issue; email flags match a Ticket run (experimental) -thirdshift process check <name> # validate one and print its plan (experimental) +thirdshift process check <name> [issue <issue>] # validate one and print its plan (experimental) thirdshift score # sample merged factory pull requests (see Scoring runs) thirdshift scores [<pull request>] [--json] # show Scores and calibration thirdshift scores <pull request> --mine [--json] # enter your five Scores @@ -767,8 +767,9 @@

Custom processes

A Custom process runs Steps in order, and can run consecutive Steps together. -A Step is a skill session or a command. It is experimental. Write -~/.thirdshift/processes/<name>.toml. The name is the file's stem.

+A Step is a skill session, a command, or the catalogue Step delivery. It is +experimental. Write ~/.thirdshift/processes/<name>.toml. The name is the +file's stem.

version = 1
 description = "Gather notes, then review them"
 
@@ -827,6 +828,54 @@
 skill = "thirdshift-to-spec"
 worktree = "judge"
+

One worktree may instead be an Issue branch, and one Step may publish it:

+
version = 1
+description = "Ship a fix"
+
+[worktrees.fix]
+kind = "issue-branch"
+
+[[steps]]
+name = "implement"
+skill = "thirdshift-implement"
+worktree = "fix"
+
+[[steps]]
+name = "ship"
+catalogue = "delivery"
+worktree = "fix"
+
+
thirdshift process check ship issue 7
+thirdshift process ship issue 7
+
+

A declared worktree's kind is throwaway or issue-branch. A throwaway +worktree is detached at the Base branch. An Issue-branch worktree is the +Issue branch a Ticket run would use for the issue the command names: +issue-<n>, then issue-<n>-branch-<k> for k ≥ 2. A Continuation reuses +the existing branch, and an open pull request's base is the Base branch. A +Process has at most one Issue-branch worktree. The Steps that name it run in +that checkout. Any other declared worktree stays throwaway.

+

catalogue = "delivery" publishes that Issue branch. It is Delivery with no +opening session and no Security review: push, open a draft pull request when +the branch has none, write the Built-with line, mark the pull request ready +for review, and keep it mergeable with green or absent CI through the Repair +loop. It does not Self-merge, even when the User config asks Ticket runs to. +When thirdshift opens the pull request, its title is the issue's and its body +starts Closes #<n>. Repairs run on the Command's Harness, Model and Effort. +A Process has at most one delivery Step. It names the Issue-branch worktree, +it is not optional, and it is not in a parallel group. It comes after the +last Step that uses that worktree. The only catalogue value is delivery.

+

An Issue-branch worktree needs an issue. thirdshift process <name> and +thirdshift process check <name> refuse the file before any session, any +Claim and any branch, naming the issue rule and the file line. With an +issue, the run makes the Claim when it starts, labelling the issue +in-progress in place of ready-for-agent, and holds it until the end. A +pull request left ready for review removes the Claim and does not put +ready-for-agent back, so Pickup does not take an issue that already has an +open pull request. A Step that fails before Delivery keeps the Issue-branch +work as a Failed ticket run does, and the failure says where it stopped, what +was preserved and the command thirdshift process <name> issue <url>. The +Claim stays when that work reached origin, and is released when it did not.

version must be 1. Each Step has a name and exactly one kind. skill is looked up the same way as thirdshift skill. First a Factory skill whose directory is exactly that name. Then the repository's committed @@ -840,9 +889,8 @@ (ADR-0012), under its own name, as a copy written out beside those skills, and kept out of git. A repository skill of the same name is not replaced. command is a command or script, -run with sh -c in the Step's worktree. It starts no session. A declared -worktree's kind must be throwaway: it is detached at the Base branch. When -the Process declares worktrees, every Step names one with worktree. +run with sh -c in the Step's worktree. It starts no session. When the +Process declares worktrees, every Step names one with worktree. Consecutive Steps with the same parallel name run at the same time. Every session or command Step edits the worktree it runs in, so Steps in one group cannot share a worktree. The same group name after a Step that is not in the @@ -851,22 +899,33 @@ failure stops the Process once the other Steps in its group have finished.

thirdshift process
 thirdshift process check gather-review
+thirdshift process check ship issue 7
 thirdshift process gather-review
+thirdshift process ship issue 7

thirdshift process lists each Custom process and its description. check validates the file and prints the plan: the description, the Command's Harness, Model and Effort, and each Step. A Process with no named worktrees prints one throwaway worktree at the Base branch. A Process that declares worktrees prints each one, the worktree each Step uses, and a together: line for each group -of Steps that run at the same time. A skill Step shows the skill and where it -was found. A command Step shows its command, and check does not run it. An -optional Step is marked (optional). check starts no session.

+of Steps that run at the same time. A throwaway worktree is described as +throwaway at the Base branch. An Issue-branch worktree is described as an +Issue branch. A skill Step shows the skill and where it was found. A command +Step shows its command, and check does not run it. A delivery Step shows +delivery. An optional Step is marked (optional). check starts no +session. check <name> issue <issue> resolves the issue and still creates no +Issue branch and makes no Claim. Without an issue, an Issue-branch worktree +is the refusal above, not a plan.

Running the Process starts the Steps in its throwaway worktrees, detached at the Base branch and cleaned up as a standalone skill's worktree is. Every throwaway worktree is removed at the end, including after a failure, unless uncommitted changes, commits absent from every remote-tracking branch, or -uncertain ownership keep it. The Base branch is the branch checked out in the -launch directory, at its commit on origin. The launch checkout stays untouched.

+uncertain ownership keep it. The Issue-branch worktree is a real checkout of +the Issue branch, beside the launch repository, and is removed at the end +when its work is on origin, as a Ticket run's is. The Base branch is the +branch checked out in the launch directory, at its commit on origin, or the +open pull request's base in a Continuation. The launch checkout stays +untouched.

Every session Step uses the Command's Harness, Model and Effort: the User config default, or the Harness default when that is unset. A command Step does not. Each finished Step's result is stored beside the Command log, one file @@ -898,14 +957,19 @@ Trusted account, made since a few minutes before the Process started, fails that Step instead of starting the session. The Command log names the cause.

check and running refuse a missing file, a file that is not valid TOML, a -missing or wrong version, an unknown key, a Step without exactly one of -skill and command, a command that is not a string or is empty, an -optional value that is not a boolean, a skill that cannot be found, an -unknown worktree, a Step that does not name a worktree when the Process -declares them, parallel Steps that share a worktree, a worktree whose kind is -not throwaway, and a reserved name (check, list, ticket-run, -spec-run, architect-run, pickup-run, security-run). The refusal names -the rule and the file line, and starts no session.

+missing or wrong version, an unknown key, a Step without exactly one kind, +a command that is not a string or is empty, an optional value that is not a +boolean, a skill that cannot be found, an unknown worktree, a Step that does +not name a worktree when the Process declares them, parallel Steps that share +a worktree, a worktree kind other than throwaway or issue-branch, a +second Issue-branch worktree, a catalogue value other than delivery, a +delivery Step with no Issue-branch worktree or on a different worktree, a +second delivery Step, an optional delivery Step, a delivery Step in a +parallel group, an Issue-branch worktree with no delivery Step after the last +Step that uses it, an Issue-branch worktree with no issue, and a reserved +name (check, list, ticket-run, spec-run, architect-run, +pickup-run, security-run). The refusal names the rule and the file line, +and starts no session.

The Command is recorded as kind Process, with the process name. Its Command log has one line per Step: the Step's name, duration, outcome and cause. A command that exits non-zero is failed, and the line names its exit status. @@ -921,7 +985,9 @@ notification under the same email, email-unless-merged and no-email flags and the same User config rules as a Ticket run. A missing address or Resend key stops the Process before any Step. A skipped Process sends none.

-

A Custom process started with no issue is a Pass. While another Pass on the repository is still running, its Spec run or Ticket run included, thirdshift process <name> is skipped: it prints another Pass is already running on <owner>/<repo>, prints nothing on stdout, exits 0 and sends no Run notification. It creates no worktree and starts no Step. While it runs, a Pickup run on the same repository is skipped. It does not take an Architect run's Ready-issue or Architect-idea gates, so a Ready issue or an Architect idea waiting for triage does not skip it. The Command record's outcome is skipped, and its cause is that reason. The Activity log records Process <name> skipped: <reason> when that is not already the last Process line. With activity.quiet_skips set, a skipped Process prints nothing. thirdshift process <name> issue <issue> is not a Pass: this rule does not skip it, and it does not hold the lock.

+

A Custom process started with no issue is a Pass, except one +that declares an Issue-branch worktree: that is refused, as above, rather than +skipped. While another Pass on the repository is still running, its Spec run or Ticket run included, thirdshift process <name> is skipped: it prints another Pass is already running on <owner>/<repo>, prints nothing on stdout, exits 0 and sends no Run notification. It creates no worktree and starts no Step. While it runs, a Pickup run on the same repository is skipped. It does not take an Architect run's Ready-issue or Architect-idea gates, so a Ready issue or an Architect idea waiting for triage does not skip it. The Command record's outcome is skipped, and its cause is that reason. The Activity log records Process <name> skipped: <reason> when that is not already the last Process line. With activity.quiet_skips set, a skipped Process prints nothing. thirdshift process <name> issue <issue> is not a Pass: this rule does not skip it, and it does not hold the lock.

User config
diff --git a/site/how-it-works/index.html b/site/how-it-works/index.html index 349e009d..31514ccc 100644 --- a/site/how-it-works/index.html +++ b/site/how-it-works/index.html @@ -211,7 +211,7 @@

How thirdshift works

A direct Issue URL starts work you have already chosen. Other commands find work, then dispatch the same Ticket run or Spec run you just followed. Each invocation makes one pass; your operating system's scheduler starts the next one.

To run one skill directly, use thirdshift skill <skill> [issue <issue>]. It starts one headless session on your default Harness, Model and Effort in a temporary worktree at your checkout's committed HEAD, and prints the final message. Worktrees with local work are kept. Standalone skills.

-

thirdshift process is experimental, beside thirdshift skill. It runs the Steps in ~/.thirdshift/processes/<name>.toml, a skill session or a command. Consecutive Steps can run together, each in a throwaway worktree the file names, and the last finished Step's result is printed. An optional Step can fail without stopping the process. thirdshift process <name> issue <issue> gives the process an issue, so each session Step works from that issue. With no issue it is a Pass and skips while another Pass on the repository is running, without an Architect run's Ready-issue or Architect-idea gates. With an issue, it is not a Pass. Status, the Console and the Activity log list it as Process <name>, and a failed one is in Needs you. It sends a Run notification under the same email rules as a Ticket run, except a skipped Process sends none. Custom processes.

+

thirdshift process is experimental, beside thirdshift skill. It runs the Steps in ~/.thirdshift/processes/<name>.toml, a skill session, a command, or a delivery Step. Consecutive Steps can run together, each in a throwaway worktree the file names, and the last finished Step's result is printed. An optional Step can fail without stopping the process. thirdshift process <name> issue <issue> gives the process an issue, so each session Step works from that issue. One Issue-branch worktree plus delivery publishes a ready-for-review pull request from that Issue branch and then removes the Claim. With no issue, and no Issue-branch worktree, it is a Pass and skips while another Pass on the repository is running, without an Architect run's Ready-issue or Architect-idea gates. With an issue, it is not a Pass. Status, the Console and the Activity log list it as Process <name>, and a failed one is in Needs you. It sends a Run notification under the same email rules as a Ticket run, except a skipped Process sends none. Custom processes.

Different entry points, the same implementation and Delivery
diff --git a/site/index.html b/site/index.html index d7f25a1f..1d1364af 100644 --- a/site/index.html +++ b/site/index.html @@ -91,7 +91,7 @@

Your issues become pull requests overnight.

  1. /thirdshift-implement
  2. /thirdshift-code-review
  3. /thirdshift-pr

thirdshift takes an Issue URL: one issue starts a Ticket run; an issue with sub-issues starts a Spec run. Its Factory skills, adapted to run with no human in the loop, implement the issue with /thirdshift-implement, review the work with /thirdshift-code-review and write the pull request with /thirdshift-pr.

Issues labelled bug first use /thirdshift-diagnosing-bugs to reproduce the reported failure and write a failing regression test before fixing it. If it cannot reproduce the bug, the Ticket run leaves triage notes and needs-info on the issue, ending successfully without a new pull request. Security fixes and Base fixes keep their own flow.

-

thirdshift skill runs one skill. thirdshift process is experimental: it runs skill or command Steps from ~/.thirdshift/processes, including Steps that run together in throwaway worktrees the file names. An optional Step can fail without stopping the process. Pass issue to give the process an issue, so each session Step works from that issue. With no issue it is a Pass: it skips, exiting 0 with no Run notification, while another Pass on the repository is running, and it does not take an Architect run's Ready-issue or Architect-idea gates. With an issue, it is not a Pass. Status, the Console and the Activity log list it as Process <name>, and a failed one is in Needs you. It sends a Run notification under the same email rules as a Ticket run, except a skipped Process sends none. Custom processes.

+

thirdshift skill runs one skill. thirdshift process is experimental: it runs skill or command Steps from ~/.thirdshift/processes, including Steps that run together in throwaway worktrees the file names. An optional Step can fail without stopping the process. Pass issue to give the process an issue, so each session Step works from that issue. A file can name one Issue-branch worktree and a delivery Step: the run holds the Claim, and Delivery leaves a ready-for-review pull request and then removes the Claim. With no issue, and no Issue-branch worktree, it is a Pass: it skips, exiting 0 with no Run notification, while another Pass on the repository is running, and it does not take an Architect run's Ready-issue or Architect-idea gates. With an issue, it is not a Pass. Status, the Console and the Activity log list it as Process <name>, and a failed one is in Needs you. It sends a Run notification under the same email rules as a Ticket run, except a skipped Process sends none. Custom processes.

Read every prompt and skill the agents get →

diff --git a/src/args.rs b/src/args.rs index d92f09ad..2270a900 100644 --- a/src/args.rs +++ b/src/args.rs @@ -20,7 +20,10 @@ use crate::run::Goal; /// What `thirdshift process` was asked to do. pub enum ProcessAction { List, - Check(String), + Check { + name: String, + issue: Option, + }, Run { name: String, issue: Option, @@ -837,7 +840,7 @@ fn metrics(args: &[String]) -> Result { Ok(asked) } -const PROCESS_USAGE: &str = "usage: thirdshift process [check | [issue ] [email [address] | email-unless-merged [address] | no-email]]"; +const PROCESS_USAGE: &str = "usage: thirdshift process [check [issue ] | [issue ] [email [address] | email-unless-merged [address] | no-email]]"; fn parse_process(args: &[String]) -> Result { if args.is_empty() { @@ -860,7 +863,7 @@ fn parse_process(args: &[String]) -> Result { continue; } if arg == "issue" { - if check || name.is_none() || expect_issue || issue.is_some() { + if name.is_none() || expect_issue || issue.is_some() { bail!(PROCESS_USAGE); } expect_issue = true; @@ -880,8 +883,8 @@ fn parse_process(args: &[String]) -> Result { bail!(PROCESS_USAGE); } match (check, name) { - (true, Some(name)) if issue.is_none() => Ok(ProcessAction::Check(name)), - (true, _) => bail!(PROCESS_USAGE), + (true, Some(name)) => Ok(ProcessAction::Check { name, issue }), + (true, None) => bail!(PROCESS_USAGE), (false, Some(name)) => Ok(ProcessAction::Run { name, issue, email }), (false, None) => bail!(PROCESS_USAGE), } diff --git a/src/claim.rs b/src/claim.rs index 3f6e8ae7..744fbefb 100644 --- a/src/claim.rs +++ b/src/claim.rs @@ -5,7 +5,9 @@ //! issue closed, kept while its pull request waits for review, and released //! when the run failed with nothing on origin to take over, except a failed //! Security fix, which stays with the Day shift. A Stop instead leaves the -//! issue for triage, or explicitly releases it when asked to requeue. +//! issue for triage, or explicitly releases it when asked to requeue. A +//! Custom process's Delivery removes it when the pull request is ready for +//! review, without putting `ready-for-agent` back. //! //! What it reads and changes of GitHub and origin, whether the run is //! interrupted, and its progress lines all go through [`Outside`]: @@ -99,6 +101,20 @@ impl Claim<'_> { reached, ); } + + /// Take `in-progress` off the issue, whether or not it is open, and put + /// nothing back. A Delivery that left a pull request ready for review + /// ends this way: the issue is not Ready, so Pickup does not take it + /// again while that pull request is open. A failure is a warning naming + /// what to run by hand. + pub fn remove(self) { + let launch = self.launch.completion(); + self.claimed.remove(&mut OnGitHub { + launch: &launch, + github: GitHub::new().completion(), + branch: &self.branch, + }); + } } /// What the Claim reads and changes of its issue on GitHub and of origin, @@ -382,6 +398,34 @@ impl<'a> Claimed<'a> { (off, on) } + /// Take `in-progress` off, open issue or not, and put nothing on. An issue + /// that is no longer `in-progress` is left as it is. + fn remove(&self, outside: &mut impl Outside) { + if let Err(error) = retry_if_interrupted(outside, |outside| self.take_off(outside)) { + outside.warn( + &error, + format!( + "could not remove {IN_PROGRESS} from issue #{}, so remove it by hand: {}", + self.issue.number, + labels::by_hand(self.issue, &[IN_PROGRESS], &[]) + ), + ); + } + } + + /// [`Claimed::remove`], its failure as `gh` gave it. + fn take_off(&self, outside: &mut impl Outside) -> Result<()> { + let edit = Edit::of(self.issue, outside.labels(self.issue)?, &[IN_PROGRESS], &[]); + if !edit.takes_off(IN_PROGRESS) { + return Ok(()); + } + outside.step(format!( + "removing {IN_PROGRESS} from issue #{}", + self.issue.number + )); + outside.apply(&edit) + } + /// Remove the Claim if the Self-merge has left its issue closed: /// `in-progress` comes off, in one request that keeps the issue's other /// labels. An issue still open, as when closing it failed, keeps it, and diff --git a/src/delivery.rs b/src/delivery.rs index 256e4eb7..241db8a1 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -199,6 +199,113 @@ impl Delivery<'_> { crate::logs::unaddressed_findings(pull_request.unaddressed_findings()); result } + + /// Publish the Issue branch already produced by a Custom process and + /// leave its pull request ready for review. There is no opening session + /// and no Security review: the Process's Steps did the work. Push, open + /// a draft pull request when the branch has none, write the Built-with + /// line, mark it ready, and keep it mergeable and its CI green through + /// the Repair loop. The goal is ready for review, so this does not + /// Self-merge. Repairs run on [`Delivery::child_asks`]'s Harness. Any + /// failure goes through the Failed run path. + pub fn publish( + self, + worktree: Worktree, + pull_request: &mut PullRequest, + ) -> Result { + let tracker = Tracker::new(Stage::Push, CommandLog); + pull_request.begin_delivery(); + let (issue, started, path) = (self.issue, self.started, worktree.path().to_path_buf()); + let before_each = move || { + no_untrusted_edit_since(issue, started)?; + hand_over_issue(issue, &path) + }; + let (delivered, log) = Sessions::checked_within( + self.logs, + worktree.path(), + &self.child_asks.harness, + self.child_asks.security_review_harness.as_ref(), + self.child_asks.security_review_default.as_ref(), + &before_each, + |sessions| { + publish_steps( + &tracker, + self.issue, + self.base, + self.child_asks, + self.base_fix, + &worktree, + pull_request, + sessions, + ) + }, + ); + let result = match delivered { + Ok(url) => Ok(url), + Err(error) => { + if let Some(merged) = error.downcast_ref::() + && worktree.fully_landed_at(&merged.head).unwrap_or(false) + { + progress::step("the pull request was merged while Delivery was running"); + Ok(merged.pr.url.clone()) + } else { + let stopped_at = stopped_at(&tracker, &error); + let path = worktree.path().to_path_buf(); + let mut outside = OnFailure { + base: self.base, + worktree, + pull_request, + }; + Err(fail(&mut outside, stopped_at, path, log, error)) + } + } + }; + crate::logs::unaddressed_findings(pull_request.unaddressed_findings()); + result + } +} + +/// [`Delivery::publish`]'s steps: push, open a draft when none exists, write +/// what it was built with, mark the pull request ready, and take it to ready +/// for review through the Repair loop. +fn publish_steps<'a>( + tracker: &Tracker, + issue: &'a IssueUrl, + base: &'a str, + child_asks: &'a ChildAsks, + base_fix: &'a mut BaseFix, + worktree: &'a Worktree, + pull_request: &'a mut PullRequest, + sessions: &'a Sessions<'a>, +) -> Result { + tracker.enter(Stage::Push); + pull_request.ensure_actionable(false)?; + worktree.push_if_actionable(&mut || pull_request.ensure_actionable(false))?; + let title = GitHub::new().issue_title(issue)?; + let body = format!("Closes #{}", issue.number); + pull_request.open_if_absent(&title, &body)?; + if let Err(error) = pull_request.write_built_with(&child_asks.harness) { + progress::warn( + &error, + format_args!("could not write the Built-with line in the pull request's body"), + ); + } + tracker.enter(Stage::PullRequestReadiness); + let pr = pull_request.mark_ready(None)?; + let mut outside = RunOutside { + tracker, + issue, + worktree, + base, + pr_url: &pr.url, + base_fix, + child_asks, + sessions, + pull_request, + ci_watch: ci::Watch::new(issue), + }; + repair_loop::take_to_goal(&mut outside, base, Goal::ReadyForReview)?; + Ok(pr.url) } /// The steps of a Delivery of the pull request for `issue`, from `branch` diff --git a/src/main.rs b/src/main.rs index f4fbf9b3..df1afead 100644 --- a/src/main.rs +++ b/src/main.rs @@ -92,8 +92,8 @@ usage: thirdshift Run the factory on the iss thirdshift secure base Use as the Base branch thirdshift skill [issue ] Run one skill and print its final message thirdshift process List Custom processes (experimental) - thirdshift process [issue ] Run a Custom process; email flags match a Ticket run (experimental) - thirdshift process check Validate a Custom process and print its plan (experimental) + thirdshift process [issue ] Run a Custom process, optionally on an issue; email flags match a Ticket run (experimental) + thirdshift process check [issue ] Validate a Custom process and print its plan (experimental) thirdshift score Sample merged factory work and propose Bug origins thirdshift scores [] [--mine] [--json] Show Scores; --mine enters yours thirdshift bug-origins [] [--json] Show origins; --confirm / --correct / --none settles one diff --git a/src/processes.rs b/src/processes.rs index c32b5651..0e019af0 100644 --- a/src/processes.rs +++ b/src/processes.rs @@ -1,12 +1,18 @@ -//! Custom processes: skill and command Steps from `~/.thirdshift/processes/.toml`. +//! Custom processes: skill, command and catalogue Steps from +//! `~/.thirdshift/processes/.toml`. //! //! Steps run in order. Consecutive Steps that name the same `parallel` group //! run at the same time, each in a throwaway worktree the Process declares. +//! One worktree may be an Issue branch. The Steps that use it are the editing +//! Steps, and a `delivery` Step after the last of them publishes that branch +//! and leaves a pull request ready for review. //! -//! A Process started with no issue is a Pass. It takes the repository's Pass -//! lock and is skipped, before any worktree, while another Pass holds it. It -//! does not take an Architect run's Ready-issue or Architect-idea gates. A -//! Process started with an issue is not a Pass and does not take the lock. +//! A Process started with no issue is a Pass, unless it declares an +//! Issue-branch worktree, which needs an issue. A Pass takes the repository's +//! Pass lock and is skipped, before any worktree, while another Pass holds +//! it. It does not take an Architect run's Ready-issue or Architect-idea +//! gates. A Process started with an issue is not a Pass and does not take the +//! lock. use std::collections::BTreeSet; use std::fmt::Write; @@ -23,16 +29,24 @@ use toml_edit::{Document, Item, Key, Table}; use crate::args::ProcessAction; use crate::asks::Flags; +use crate::base_fix::{BaseFix, BaseFixAsk}; +use crate::branch::{self, Selection}; +use crate::child_run::ChildAsks; +use crate::claim::{self, Claim}; use crate::config::UserConfig; use crate::git::Git; +use crate::github::GitHub; use crate::harness::{Asked, Choice}; use crate::issue::{IssueInput, IssueUrl}; use crate::logs::{self, Begin, Work}; use crate::notification::{About, NotificationAsk, RunNotification}; use crate::process::{self, Control, Interruption}; +use crate::pull_request::PullRequest; +use crate::recovery::{self, Preservation, Recovery}; +use crate::run::Goal; use crate::run_ending::{Account, Cause}; use crate::skill; -use crate::worktree::SkillWorktree; +use crate::worktree::{Preserved, SkillWorktree, Worktree}; use crate::{delivery, interrupt, launch, progress, prompt, session, skills, trust}; /// The issue URL when the Process has an issue; unset otherwise. @@ -60,6 +74,8 @@ struct ProcessFile { struct WorktreeDraft { name: String, + /// The `kind` line when this worktree is an Issue branch. + issue_branch_line: Option, } struct StepDraft { @@ -74,6 +90,12 @@ struct StepDraft { enum DraftKind { Skill { skill: String, line: usize }, Command { command: String }, + Catalogue { line: usize }, +} + +struct DeclaredWorktree { + name: String, + issue_branch: bool, } struct ResolvedStep { @@ -87,6 +109,7 @@ struct ResolvedStep { enum StepKind { Skill { name: String, path: PathBuf }, Command(String), + Delivery, } struct OwnedWorktree { @@ -96,7 +119,9 @@ struct OwnedWorktree { struct Prepared { worktrees: Vec, - declared: Vec, + declared: Vec, + /// The Issue-branch worktree's name, when the file declares one. + issue_worktree: Option, _written: WrittenGuard, choice: Choice, description: String, @@ -106,6 +131,43 @@ struct Prepared { started: DateTime, } +/// The Issue branch a run is delivering, beside the throwaway worktrees. +struct IssueRun<'a> { + checkout: &'a mut Option, + issue: &'a IssueUrl, + branch: &'a str, + base: &'a str, + on_origin: bool, + logs: &'a session::Logs, + base_fix: &'a mut BaseFix, + process: &'a str, +} + +/// Ends the Claim on drop unless the run removed it or ended it already. +struct HeldClaim<'a> { + claim: Option>, +} + +impl HeldClaim<'_> { + fn remove(&mut self) { + if let Some(claim) = self.claim.take() { + claim.remove(); + } + } + + fn release(&mut self) { + if let Some(claim) = self.claim.take() { + claim.end(None); + } + } +} + +impl Drop for HeldClaim<'_> { + fn drop(&mut self) { + self.release(); + } +} + /// What one Step needs from the Process, shared by a parallel group. struct StepRun<'a> { process: &'a str, @@ -125,7 +187,7 @@ impl Drop for WrittenGuard { pub fn execute(action: ProcessAction) -> Result { match action { ProcessAction::List => list(), - ProcessAction::Check(name) => check(&name), + ProcessAction::Check { name, issue } => check(&name, issue), ProcessAction::Run { name, issue, email } => run(&name, issue, email), } } @@ -168,10 +230,24 @@ fn list() -> Result { Ok(lines.join("\n")) } -fn check(name: &str) -> Result { +fn check(name: &str, issue: Option) -> Result { interrupt::install()?; let config = UserConfig::load()?; - let prepared = prepare(name, &config)?; + let (path, file) = load(name)?; + let issue = resolve_process_issue(issue)?; + require_issue(&path, &file, issue.as_ref())?; + let base = launch::checked_out_base()?; + let launch = Git::new(std::env::current_dir()?); + let prepared = prepare_from( + &path, + &config, + file, + issue, + Utc::now(), + &launch, + base.origin_commit(), + None, + )?; Ok(plan(name, &prepared)) } @@ -182,7 +258,7 @@ fn run(name: &str, issue: Option, email: Option) -> // address or Resend key fails the Process before it starts. As for the // other Passes, the check is before a skip, so a Process that would be // skipped fails on it too. - let mut notification = RunNotification::new( + let notification = RunNotification::new( Flags { email, ..Flags::default() @@ -191,14 +267,22 @@ fn run(name: &str, issue: Option, email: Option) -> &config.email, About::Process { name }, )?; + let (path, file) = match load(name) { + Ok(loaded) => loaded, + Err(error) => return Err(failed_before_steps(notification, error)), + }; // Before any worktree: a foreign Issue URL is an origin mismatch, and a - // number that cannot be resolved stops the same way. A failure here is - // still a failed Process, so it sends the Run notification a Ticket run - // would send for the same kind of stop. + // number that cannot be resolved stops the same way. An Issue-branch + // worktree with no issue is refused before the Pass log. A failure here + // is still a failed Process, so it sends the Run notification a Ticket + // run would send for the same kind of stop. let issue = match resolve_process_issue(issue) { Ok(issue) => issue, Err(error) => return Err(failed_before_steps(notification, error)), }; + if let Err(error) = require_issue(&path, &file, issue.as_ref()) { + return Err(failed_before_steps(notification, error)); + } // No issue: a Pass. The Base branch is checked before the lock, so a bad // checkout fails instead of being skipped, and a skip creates no worktree. // The lock is then held until this process exits, through every Step. @@ -207,10 +291,11 @@ fn run(name: &str, issue: Option, email: Option) -> if issue.is_none() { logs::begin(Begin::Process { name, pass: true }); logs::configured(&config); - let repo = match (|| { - let _base = launch::checked_out_base()?; - launch::repo() - })() { + let base = match launch::checked_out_base() { + Ok(base) => base, + Err(error) => return Err(failed_before_steps(notification, error)), + }; + let repo = match launch::repo() { Ok(repo) => repo, Err(error) => return Err(failed_before_steps(notification, error)), }; @@ -225,33 +310,59 @@ fn run(name: &str, issue: Option, email: Option) -> Ok(None) => {} Err(error) => return Err(failed_before_steps(notification, error)), } + let started = Utc::now() - crate::run::STARTED_MARGIN; + let launch = match std::env::current_dir() { + Ok(dir) => Git::new(dir), + Err(error) => return Err(failed_before_steps(notification, error.into())), + }; + let prepared = match prepare_from( + &path, + &config, + file, + None, + started, + &launch, + base.origin_commit(), + None, + ) { + Ok(prepared) => prepared, + Err(error) => return Err(failed_before_steps(notification, error)), + }; + return finish_run(name, &repo, prepared, notification); } - // The same margin as a Ticket run. Edits in the gap since this moment, - // or dated by a slow GitHub clock, still count. + let issue = issue.expect("an issue"); let started = Utc::now() - crate::run::STARTED_MARGIN; - let mut prepared = match prepare(name, &config) { + if file_has_issue_branch(&file) { + return run_issue_branch(name, &config, &path, file, issue, started, notification); + } + let base = match launch::checked_out_base() { + Ok(base) => base, + Err(error) => return Err(failed_before_steps(notification, error)), + }; + let launch = match std::env::current_dir() { + Ok(dir) => Git::new(dir), + Err(error) => return Err(failed_before_steps(notification, error.into())), + }; + let prepared = match prepare_from( + &path, + &config, + file, + Some(issue), + started, + &launch, + base.origin_commit(), + None, + ) { Ok(prepared) => prepared, Err(error) => return Err(failed_before_steps(notification, error)), }; - prepared.issue = issue; - prepared.started = started; let repo = match launch::repo() { Ok(repo) => repo, Err(error) => return Err(failed_before_steps(notification, error)), }; - if prepared.issue.is_some() { - logs::begin(Begin::Process { name, pass: false }); - logs::configured(&config); - } - logs::started(Work::Process { repo: &repo, name }, &prepared.choice); - notification.built_with(&prepared.choice); - let result = execute_steps(name, &prepared); - let (outcome, cause) = classify(&result); - drop(prepared); - logs::ended(outcome); - logs::complete(outcome, cause.as_deref()); - notify(notification, outcome, cause.as_deref()); - result + logs::begin(Begin::Process { name, pass: false }); + logs::configured(&config); + finish_run(name, &repo, prepared, notification) } /// A stop after the email check and before any Step is still a failed Process. @@ -293,12 +404,147 @@ fn notify(notification: RunNotification, outcome: &'static str, cause: Option<&s notification.finish(&account); } -fn execute_steps(process: &str, prepared: &Prepared) -> Result { +fn finish_run( + name: &str, + repo: &crate::issue::Repo, + prepared: Prepared, + mut notification: RunNotification, +) -> Result { + logs::started(Work::Process { repo, name }, &prepared.choice); + notification.built_with(&prepared.choice); + let result = execute_steps(name, &prepared, None); + let (outcome, cause) = classify(&result); + drop(prepared); + logs::ended(&outcome); + logs::complete(&outcome, cause.as_deref()); + notify(notification, outcome, cause.as_deref()); + result +} + +fn run_issue_branch( + name: &str, + config: &UserConfig, + path: &Path, + file: ProcessFile, + issue: IssueUrl, + started: DateTime, + mut notification: RunNotification, +) -> Result { + let base_branch = match launch::checked_out_base() { + Ok(base) => base, + Err(error) => return Err(failed_before_steps(notification, error)), + }; + let launch_git = match std::env::current_dir() { + Ok(dir) => Git::new(dir), + Err(error) => return Err(failed_before_steps(notification, error.into())), + }; + let selection = match branch::select(&launch_git, &GitHub::new(), &issue) { + Ok(selection) => selection, + Err(error) => return Err(failed_before_steps(notification, error)), + }; + let base = selection + .pr_base(None, Some(base_branch.name()), |line| progress::step(&line)) + .unwrap_or(base_branch.name()) + .to_string(); + let on_origin = matches!(selection, Selection::Continuation { .. }); + let branch = selection.branch().to_string(); + let made = match claim::make(&issue, &launch_git, &branch) { + Ok(claim) => claim, + Err(error) => return Err(failed_before_steps(notification, error)), + }; + let mut claim = HeldClaim { claim: Some(made) }; + let checkout = match &selection { + Selection::Fresh { .. } => Worktree::create_fresh(&launch_git, &issue.repo, &branch, &base), + Selection::Continuation { .. } => { + Worktree::continue_existing(&launch_git, &issue.repo, &branch, &base) + } + }; + let checkout = match checkout { + Ok(checkout) => checkout, + Err(error) => return Err(failed_before_steps(notification, error)), + }; + let commit = if base == base_branch.name() { + base_branch.origin_commit().to_string() + } else { + match launch_git.run(&["rev-parse", &format!("origin/{base}")]) { + Ok(commit) => commit, + Err(error) => return Err(failed_before_steps(notification, error)), + } + }; + let prepared = match prepare_from( + path, + config, + file, + Some(issue.clone()), + started, + &launch_git, + &commit, + Some(checkout.path()), + ) { + Ok(prepared) => prepared, + Err(error) => return Err(failed_before_steps(notification, error)), + }; + let repo = match launch::repo() { + Ok(repo) => repo, + Err(error) => return Err(failed_before_steps(notification, error)), + }; + let mut held = Some(checkout); + // Session traces ask for the log root, which exists once this command is + // configured. The Command record stays the Process; the traces are named + // for the issue. + logs::begin(Begin::Process { name, pass: false }); + logs::configured(config); + let logs = session::Logs::of_run(&issue); + let mut base_fix = BaseFix::new(None, BaseFixAsk::Forbid); + logs::started(Work::Process { repo: &repo, name }, &prepared.choice); + notification.built_with(&prepared.choice); + let result = { + let mut issue_run = IssueRun { + checkout: &mut held, + issue: &issue, + branch: &branch, + base: &base, + on_origin, + logs: &logs, + base_fix: &mut base_fix, + process: name, + }; + let result = execute_steps(name, &prepared, Some(&mut issue_run)); + if let Err(error) = &result + && let Some(checkout) = issue_run.checkout.as_mut() + { + preserve_step_failure(checkout, name, &issue, &base, &branch, on_origin, error); + } + result + }; + if result.is_ok() { + claim.remove(); + } else { + claim.release(); + } + let (outcome, cause) = classify(&result); + drop(prepared); + drop(held); + logs::ended(&outcome); + logs::complete(&outcome, cause.as_deref()); + notify(notification, outcome, cause.as_deref()); + result +} + +fn execute_steps( + process: &str, + prepared: &Prepared, + mut issue_run: Option<&mut IssueRun<'_>>, +) -> Result { let adapter = prepared.choice.harness.adapter(); for worktree in &prepared.worktrees { skills::link_into(worktree.checkout.path(), adapter)?; adapter.link_instruction_fallback(worktree.checkout.path())?; } + if let Some(checkout) = issue_run.as_ref().and_then(|run| run.checkout.as_ref()) { + skills::link_into(checkout.path(), adapter)?; + adapter.link_instruction_fallback(checkout.path())?; + } let results = logs::command_log_path() .context("a Process has no Command log")? .with_extension("steps"); @@ -308,6 +554,23 @@ fn execute_steps(process: &str, prepared: &Prepared) -> Result { let mut last = String::new(); for range in group_ranges(&prepared.steps) { let group = &prepared.steps[range.clone()]; + if group.len() == 1 && matches!(group[0].kind, StepKind::Delivery) { + let Some(issue_run) = issue_run.as_mut() else { + bail!("delivery has no Issue-branch worktree"); + }; + let started = Instant::now(); + let outcome = run_delivery(prepared, &group[0], issue_run); + finish_step( + &group[0], + started.elapsed(), + outcome, + &results, + &mut earlier, + &mut last, + )?; + continue; + } + let checkout = issue_run.as_ref().and_then(|run| run.checkout.as_ref()); let outcomes = if group.len() == 1 { let started = Instant::now(); let outcome = run_step( @@ -317,10 +580,19 @@ fn execute_steps(process: &str, prepared: &Prepared) -> Result { &earlier, &results, range.start == 0, + checkout, ); vec![(started.elapsed(), outcome)] } else { - run_parallel(process, prepared, group, &earlier, &results, range.start) + run_parallel( + process, + prepared, + group, + &earlier, + &results, + range.start, + checkout, + ) }; let mut group_error = None; for (step, (elapsed, outcome)) in group.iter().zip(outcomes) { @@ -370,11 +642,12 @@ fn run_parallel( earlier: &[(String, PathBuf)], results: &Path, group_start: usize, + checkout: Option<&Worktree>, ) -> Vec<(Duration, Result)> { thread::scope(|scope| { let mut handles = Vec::with_capacity(group.len()); for (offset, step) in group.iter().enumerate() { - let cwd = worktree_path(&prepared.worktrees, step.worktree.as_deref()).to_path_buf(); + let cwd = step_dir(prepared, step, checkout).to_path_buf(); let choice = prepared.choice.clone(); let earlier = earlier.to_vec(); let process = process.to_string(); @@ -443,6 +716,7 @@ fn run_step( earlier: &[(String, PathBuf)], results: &Path, first: bool, + checkout: Option<&Worktree>, ) -> Result { let run = StepRun { process, @@ -452,7 +726,7 @@ fn run_step( }; run_step_in( &run, - worktree_path(&prepared.worktrees, step.worktree.as_deref()), + step_dir(prepared, step, checkout), step, earlier, results, @@ -460,6 +734,118 @@ fn run_step( ) } +fn step_dir<'a>( + prepared: &'a Prepared, + step: &ResolvedStep, + checkout: Option<&'a Worktree>, +) -> &'a Path { + if let Some(checkout) = checkout + && step.worktree.is_some() + && prepared.issue_worktree.as_deref() == step.worktree.as_deref() + { + return checkout.path(); + } + worktree_path(&prepared.worktrees, step.worktree.as_deref()) +} + +fn run_delivery( + prepared: &Prepared, + step: &ResolvedStep, + issue_run: &mut IssueRun<'_>, +) -> Result { + let checkout = issue_run + .checkout + .take() + .context("the Issue-branch worktree is gone")?; + progress::step(format_args!( + "running step {} with {choice}", + step.name, + choice = prepared.choice + )); + let mut pull_request = PullRequest::new( + issue_run.issue, + issue_run.branch, + issue_run.base, + GitHub::new(), + ); + let child_asks = ChildAsks { + harness: prepared.choice.clone(), + ..ChildAsks::default() + }; + let started = prepared.started; + match (delivery::Delivery { + security_review: false, + child_asks: &child_asks, + issue: issue_run.issue, + base: issue_run.base, + goal: Goal::ReadyForReview, + base_fix: issue_run.base_fix, + logs: issue_run.logs, + started, + }) + .publish(checkout, &mut pull_request) + { + Ok(url) => Ok(url), + Err(mut failed) => { + if let Some(recovery) = &mut failed.recovery { + recovery.add_command_context(continuation(issue_run), issue_run.on_origin); + for line in recovery.lines() { + progress::step(line); + } + } + Err(failed.error) + } + } +} + +fn continuation(issue_run: &IssueRun<'_>) -> recovery::Continuation { + recovery::Continuation { + command: format!( + "thirdshift process {} issue {}", + issue_run.process, issue_run.issue.url + ), + launch: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + base: issue_run.base.to_string(), + branch: issue_run.branch.to_string(), + } +} + +fn preserve_step_failure( + checkout: &mut Worktree, + process: &str, + issue: &IssueUrl, + base: &str, + branch: &str, + on_origin: bool, + error: &anyhow::Error, +) { + let reason = Cause::of(error).first_line().to_string(); + let preservation = match checkout.preserve_failed_run(base, &reason, &mut || Ok(())) { + Ok(Preserved::Pushed) => Preservation::Remote, + Ok(Preserved::NoChanges) => Preservation::None, + Err(problem) => { + progress::step(format!( + "could not push the failed ticket run's work, so it may exist only locally: {problem:#}" + )); + Preservation::Local + } + }; + let mut recovery = Recovery::new(recovery::Stage::Implementation, preservation); + recovery.record_worktree(checkout.path().to_path_buf()); + recovery.add_command_context( + recovery::Continuation { + command: format!("thirdshift process {process} issue {}", issue.url), + launch: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + base: base.to_string(), + branch: branch.to_string(), + }, + on_origin, + ); + for line in recovery.lines() { + progress::step(line); + } +} + fn run_step_in( run: &StepRun<'_>, cwd: &Path, @@ -494,6 +880,7 @@ fn run_step_in( } } } + StepKind::Delivery => bail!("delivery cannot be in a parallel group"), StepKind::Command(command) => { progress::step(format_args!("running step {}", step.name)); let result_file = results.join(format!("{}.txt", step.name)); @@ -679,7 +1066,15 @@ fn plan(name: &str, prepared: &Prepared) -> String { chosen = prepared.choice.chosen_by, ); for worktree in &prepared.declared { - let _ = writeln!(text, "worktree {worktree}: throwaway at the Base branch"); + if worktree.issue_branch { + let _ = writeln!(text, "worktree {}: Issue branch", worktree.name); + } else { + let _ = writeln!( + text, + "worktree {}: throwaway at the Base branch", + worktree.name + ); + } } let named = !prepared.declared.is_empty(); for range in group_ranges(&prepared.steps) { @@ -706,6 +1101,7 @@ fn step_plan(step: &ResolvedStep, named: bool) -> String { let shown = command.replace(['\n', '\r'], " "); format!("{}: command {shown}", step.name) } + StepKind::Delivery => format!("{}: delivery", step.name), }; if named && let Some(worktree) = &step.worktree { let _ = write!(line, " (worktree {worktree})"); @@ -716,44 +1112,84 @@ fn step_plan(step: &ResolvedStep, named: bool) -> String { line } -fn prepare(name: &str, config: &UserConfig) -> Result { +fn load(name: &str) -> Result<(PathBuf, ProcessFile)> { let path = process_path(name)?; refuse_reserved(&path, name)?; let text = read_process(&path)?; let file = parse(&path, &text)?; refuse_worktrees(&path, &file)?; - let base = launch::checked_out_base()?; - let launch = Git::new(std::env::current_dir()?); - let commit = base.origin_commit(); - let written = WrittenGuard; - let worktrees = create_worktrees(&launch, commit, &file)?; + refuse_issue_branch(&path, &file)?; + Ok((path, file)) +} + +fn prepare_from( + path: &Path, + config: &UserConfig, + file: ProcessFile, + issue: Option, + started: DateTime, + launch: &Git, + commit: &str, + issue_checkout: Option<&Path>, +) -> Result { + let issue_worktree = file + .worktrees + .iter() + .find_map(|worktree| worktree.issue_branch_line.map(|_| worktree.name.clone())); + let declared = file + .worktrees + .iter() + .map(|worktree| DeclaredWorktree { + name: worktree.name.clone(), + issue_branch: worktree.issue_branch_line.is_some(), + }) + .collect(); + let skip = issue_checkout.and(issue_worktree.as_deref()); + let worktrees = create_worktrees(launch, commit, &file, skip)?; let mut steps = Vec::new(); for step in &file.steps { - let checkout = worktree_path( - &worktrees, - step.worktree.as_ref().map(|(name, _)| name.as_str()), - ); - steps.push(resolve(&path, step, checkout)?); + let checkout = resolve_dir(&worktrees, step, issue_worktree.as_deref(), issue_checkout); + steps.push(resolve(path, step, checkout)?); } let mut choice = Choice::of(&Asked::default(), &config.harness); choice.check_without_probe()?; Ok(Prepared { worktrees, - declared: file - .worktrees - .into_iter() - .map(|worktree| worktree.name) - .collect(), - _written: written, + declared, + issue_worktree, + _written: WrittenGuard, choice, description: file.description, steps, - issue: None, - started: Utc::now(), + issue, + started, }) } -fn create_worktrees(launch: &Git, commit: &str, file: &ProcessFile) -> Result> { +fn resolve_dir<'a>( + worktrees: &'a [OwnedWorktree], + step: &StepDraft, + issue_worktree: Option<&str>, + issue_checkout: Option<&'a Path>, +) -> &'a Path { + if let Some(checkout) = issue_checkout + && step.worktree.as_ref().map(|(name, _)| name.as_str()) == issue_worktree + && issue_worktree.is_some() + { + return checkout; + } + worktree_path( + worktrees, + step.worktree.as_ref().map(|(name, _)| name.as_str()), + ) +} + +fn create_worktrees( + launch: &Git, + commit: &str, + file: &ProcessFile, + skip: Option<&str>, +) -> Result> { if file.worktrees.is_empty() { return Ok(vec![OwnedWorktree { name: None, @@ -766,7 +1202,7 @@ fn create_worktrees(launch: &Git, commit: &str, file: &ProcessFile) -> Result Result<()> { Ok(()) } +fn file_has_issue_branch(file: &ProcessFile) -> bool { + file.worktrees + .iter() + .any(|worktree| worktree.issue_branch_line.is_some()) +} + +fn require_issue(path: &Path, file: &ProcessFile, issue: Option<&IssueUrl>) -> Result<()> { + if issue.is_some() { + return Ok(()); + } + let Some(worktree) = file + .worktrees + .iter() + .find(|worktree| worktree.issue_branch_line.is_some()) + else { + return Ok(()); + }; + Err(rule( + path, + worktree.issue_branch_line.expect("issue-branch line"), + "issue", + "an Issue-branch worktree needs an issue", + )) +} + +fn refuse_issue_branch(path: &Path, file: &ProcessFile) -> Result<()> { + let mut issue_branches = file + .worktrees + .iter() + .filter(|worktree| worktree.issue_branch_line.is_some()); + let Some(issue_branch) = issue_branches.next() else { + for step in &file.steps { + if let DraftKind::Catalogue { line } = &step.kind { + return Err(rule( + path, + *line, + "delivery", + "delivery needs an Issue-branch worktree", + )); + } + } + return Ok(()); + }; + if let Some(second) = issue_branches.next() { + return Err(rule( + path, + second.issue_branch_line.expect("issue-branch line"), + "issue-branch", + "a Process has at most one Issue-branch worktree", + )); + } + let mut delivery_at = None; + let mut last_use = None; + for (index, step) in file.steps.iter().enumerate() { + if let DraftKind::Catalogue { line } = &step.kind { + if delivery_at.is_some() { + return Err(rule( + path, + *line, + "delivery", + "a Process has at most one delivery Step", + )); + } + if step.optional { + return Err(rule(path, *line, "delivery", "delivery cannot be optional")); + } + if let Some((_, parallel_line)) = &step.parallel { + return Err(rule( + path, + *parallel_line, + "delivery", + "delivery cannot be in a parallel group", + )); + } + let uses = step + .worktree + .as_ref() + .is_some_and(|(name, _)| name == &issue_branch.name); + if !uses { + let line = step + .worktree + .as_ref() + .map(|(_, line)| *line) + .unwrap_or(*line); + return Err(rule( + path, + line, + "delivery", + "delivery must use the Issue-branch worktree", + )); + } + delivery_at = Some(index); + } else if step + .worktree + .as_ref() + .is_some_and(|(name, _)| name == &issue_branch.name) + { + last_use = Some(index); + } + } + match last_use { + Some(use_at) if delivery_at.is_some_and(|at| at > use_at) => Ok(()), + Some(use_at) => { + let step = &file.steps[use_at]; + let line = step + .worktree + .as_ref() + .map(|(_, line)| *line) + .unwrap_or(step.name_line); + Err(rule( + path, + line, + "delivery", + format!( + "no delivery Step after step {}, which uses the Issue-branch worktree", + step.name + ), + )) + } + None if delivery_at.is_some() => Ok(()), + None => Err(rule( + path, + issue_branch.issue_branch_line.expect("issue-branch line"), + "delivery", + "an Issue-branch worktree needs a delivery Step after the last Step that uses it", + )), + } +} + fn resolve(path: &Path, step: &StepDraft, worktree: &Path) -> Result { let kind = match &step.kind { DraftKind::Skill { skill, line } => match skills::find(worktree, skill) { @@ -858,6 +1423,7 @@ fn resolve(path: &Path, step: &StepDraft, worktree: &Path) -> Result StepKind::Command(command.clone()), + DraftKind::Catalogue { .. } => StepKind::Delivery, }; Ok(ResolvedStep { name: step.name.clone(), @@ -998,6 +1564,7 @@ fn parse_worktrees( )); }; let mut saw_kind = false; + let mut issue_branch_line = None; for (field, value) in body.iter() { let field_line = key_line(text, body.key(field)); match field { @@ -1010,15 +1577,16 @@ fn parse_worktrees( "kind must be a string", )); }; - if kind != "throwaway" { + if kind != "throwaway" && kind != "issue-branch" { return Err(rule( path, field_line, "worktree kind", - "kind must be throwaway", + "kind must be throwaway or issue-branch", )); } saw_kind = true; + issue_branch_line = (kind == "issue-branch").then_some(field_line); } other => return Err(rule(path, field_line, "unknown key", other)), } @@ -1028,6 +1596,7 @@ fn parse_worktrees( } worktrees.push(WorktreeDraft { name: key.to_string(), + issue_branch_line, }); } Ok(worktrees) @@ -1093,8 +1662,7 @@ fn parse_steps(path: &Path, text: &str, line: usize, item: &Item) -> Result Result { let mut name = None; - let mut skill = None; - let mut command = None; + let mut kinds = Vec::new(); let mut worktree = None; let mut parallel = None; let mut optional = false; @@ -1102,8 +1670,18 @@ fn parse_step(path: &Path, text: &str, table_line: usize, table: &Table) -> Resu let line = key_line(text, table.key(key)); match key { "name" => name = Some(step_name(path, line, item)?), - "skill" => skill = Some(step_skill(path, line, item)?), - "command" => command = Some(step_command(path, line, item)?), + "skill" => { + let (skill, line) = step_skill(path, line, item)?; + kinds.push((DraftKind::Skill { skill, line }, line)); + } + "command" => { + let (command, line) = step_command(path, line, item)?; + kinds.push((DraftKind::Command { command }, line)); + } + "catalogue" => { + let line = step_catalogue(path, line, item)?; + kinds.push((DraftKind::Catalogue { line }, line)); + } "worktree" => worktree = Some(step_worktree(path, line, item)?), "parallel" => parallel = Some(step_parallel(path, line, item)?), "optional" => optional = step_optional(path, line, item)?, @@ -1113,25 +1691,21 @@ fn parse_step(path: &Path, text: &str, table_line: usize, table: &Table) -> Resu let Some((name, name_line)) = name else { return Err(rule(path, table_line, "step name", "a step needs a name")); }; - let kind = match (skill, command) { - (Some((skill, line)), None) => DraftKind::Skill { skill, line }, - (None, Some((command, _))) => DraftKind::Command { command }, - (None, None) => { - return Err(rule( - path, - name_line, - "exactly one kind", - format!("step {name} has no kind"), - )); - } - (Some(_), Some((_, line))) => { - return Err(rule( - path, - line, - "exactly one kind", - format!("step {name} has more than one kind"), - )); - } + if kinds.len() > 1 { + return Err(rule( + path, + kinds[1].1, + "exactly one kind", + format!("step {name} has more than one kind"), + )); + } + let Some((kind, _)) = kinds.pop() else { + return Err(rule( + path, + name_line, + "exactly one kind", + format!("step {name} has no kind"), + )); }; Ok(StepDraft { name, @@ -1205,6 +1779,16 @@ fn step_skill(path: &Path, line: usize, item: &Item) -> Result<(String, usize)> Ok((value.to_string(), line)) } +fn step_catalogue(path: &Path, line: usize, item: &Item) -> Result { + let Some(value) = item.as_str() else { + return Err(rule(path, line, "catalogue", "catalogue must be a string")); + }; + if value != "delivery" { + return Err(rule(path, line, "catalogue", "catalogue must be delivery")); + } + Ok(line) +} + fn step_command(path: &Path, line: usize, item: &Item) -> Result<(String, usize)> { let Some(value) = item.as_str() else { return Err(rule( diff --git a/src/pull_request.rs b/src/pull_request.rs index 162226f7..46e6d5c0 100644 --- a/src/pull_request.rs +++ b/src/pull_request.rs @@ -282,6 +282,27 @@ impl PullRequest { Ok(identified) } + /// Open a draft pull request from this Issue branch when the branch has + /// none. An existing pull request, draft or ready, is left to + /// [`PullRequest::mark_ready`]. + pub fn open_if_absent(&mut self, title: &str, body: &str) -> Result<()> { + if self.observe(false)?.is_some() { + return Ok(()); + } + progress::step(format!("opening a draft pull request into {}", self.base)); + let url = self + .adapter + .create_draft(&self.issue, &self.branch, &self.base, title, body)?; + let url = url.trim().to_string(); + let number = self.number_from_url(&url)?; + self.identified = Some(Identified { number, url }); + let snapshot = self + .observe(false)? + .context("the pull request just opened is not found")?; + self.validate(&snapshot, false)?; + Ok(()) + } + /// Immutable information for Delivery's final accounting, including /// identity captured only by Failed run completion discovery. pub fn info(&self) -> Option { diff --git a/tests/process.rs b/tests/process.rs index 474e596e..befc2aec 100644 --- a/tests/process.rs +++ b/tests/process.rs @@ -1343,7 +1343,7 @@ version = 1 description = \"Branch\" [worktrees.fix] -kind = \"issue-branch\" +kind = \"orbit\" ", ":5: worktree kind:", ), @@ -2149,7 +2149,8 @@ fn a_missing_issue_value_or_extra_argument_is_a_usage_error() { let cases: &[&[&str]] = &[ &["process", "notes", "issue"], &["process", "notes", "issue", "7", "extra"], - &["process", "check", "notes", "issue", "7"], + &["process", "check", "notes", "issue"], + &["process", "check", "notes", "issue", "7", "extra"], &["process", "issue", "7"], ]; for args in cases { @@ -3213,3 +3214,561 @@ fn an_origin_mismatch_still_sends_the_run_notification() { ); assert_eq!(resend.requests().len(), 1, "{:?}", resend.requests()); } + +const SHIP: &str = "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[[steps]] +name = \"implement\" +skill = \"code-review\" +worktree = \"fix\" + +[[steps]] +name = \"review\" +skill = \"code-review\" +worktree = \"fix\" + +[[steps]] +name = \"ship\" +catalogue = \"delivery\" +worktree = \"fix\" +"; + +#[test] +fn an_issue_branch_without_a_delivery_after_its_editing_steps_is_refused() { + let body = "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[[steps]] +name = \"implement\" +skill = \"code-review\" +worktree = \"fix\" +"; + for check in [true, false] { + let scenario = Scenario::new(); + write_process(&scenario, "ship", body); + let result = if check { + scenario.run(&["process", "check", "ship"]) + } else { + scenario.run(&["process", "ship", "issue", "7"]) + }; + assert_no_session(&scenario, &result); + assert!( + result.stderr.contains(":10: delivery:"), + "check={check} expected the delivery rule in {}", + result.stderr + ); + assert_eq!(scenario.issue_labels(7), Vec::::new()); + assert!(scenario.origin_log("issue-7").is_none()); + } +} + +fn refuse_issue_branch(body: &str, needle: &str) { + for check in [true, false] { + let scenario = Scenario::new(); + write_process(&scenario, "ship", body); + let result = if check { + scenario.run(&["process", "check", "ship", "issue", "7"]) + } else { + scenario.run(&["process", "ship", "issue", "7"]) + }; + assert_no_session(&scenario, &result); + assert!( + result.stderr.contains(needle), + "check={check} expected {needle} in {}", + result.stderr + ); + assert_eq!(scenario.issue_labels(7), Vec::::new()); + assert!(scenario.origin_log("issue-7").is_none()); + } +} + +#[test] +fn an_issue_branch_process_needs_an_issue() { + for check in [true, false] { + let scenario = Scenario::new(); + write_process(&scenario, "ship", SHIP); + let result = if check { + scenario.run(&["process", "check", "ship"]) + } else { + scenario.run(&["process", "ship"]) + }; + assert_no_session(&scenario, &result); + assert!( + result.stderr.contains(":5: issue:"), + "check={check} expected the issue rule in {}", + result.stderr + ); + assert!( + result + .stderr + .contains("an Issue-branch worktree needs an issue"), + "check={check} {}", + result.stderr + ); + assert_eq!(scenario.issue_labels(7), Vec::::new()); + assert!(scenario.origin_log("issue-7").is_none()); + } +} + +#[test] +fn check_of_an_issue_branch_process_prints_the_plan_and_starts_no_session() { + let scenario = Scenario::new(); + write_process(&scenario, "ship", SHIP); + scenario.issue_labelled(7, &["ready-for-agent"]); + + let result = scenario.run(&["process", "check", "ship", "issue", "7"]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert!(scenario.claude_calls().is_empty(), "a session started"); + assert!( + result.stdout.contains("ship: Ship a fix"), + "{}", + result.stdout + ); + assert!( + result.stdout.contains("worktree fix: Issue branch"), + "{}", + result.stdout + ); + assert!( + result.stdout.contains("ship: delivery (worktree fix)"), + "{}", + result.stdout + ); + assert!( + result.stdout.contains("implement: thirdshift-code-review"), + "{}", + result.stdout + ); + assert_eq!(scenario.issue_labels(7), ["ready-for-agent"]); + assert!(scenario.origin_log("issue-7").is_none()); + assert_eq!(worktree_count(&scenario), 1); + assert!(!process_dir(&scenario).exists()); + + let throwaway = Scenario::new(); + write_process(&throwaway, "notes", REVIEW); + let plan = throwaway.run(&["process", "check", "notes"]); + assert_eq!(plan.code, Some(0), "{}", plan.stderr); + assert!( + plan.stdout + .contains("throwaway worktree at the Base branch") + ); +} + +#[test] +fn an_issue_branch_process_is_refused_when_its_shape_would_lose_work() { + refuse_issue_branch( + "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[worktrees.again] +kind = \"issue-branch\" + +[[steps]] +name = \"implement\" +skill = \"code-review\" +worktree = \"fix\" + +[[steps]] +name = \"ship\" +catalogue = \"delivery\" +worktree = \"fix\" +", + ":8: issue-branch:", + ); + refuse_issue_branch( + "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[[steps]] +name = \"implement\" +skill = \"code-review\" +worktree = \"fix\" + +[[steps]] +name = \"ship\" +catalogue = \"self-merge\" +worktree = \"fix\" +", + ":14: catalogue:", + ); + refuse_issue_branch( + "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[[steps]] +name = \"ship\" +catalogue = \"delivery\" +optional = true +worktree = \"fix\" +", + ":9: delivery:", + ); + refuse_issue_branch( + "\ +version = 1 +description = \"Ship a fix\" + +[[steps]] +name = \"ship\" +catalogue = \"delivery\" +", + ":6: delivery:", + ); + refuse_issue_branch( + "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[worktrees.notes] +kind = \"throwaway\" + +[[steps]] +name = \"implement\" +skill = \"code-review\" +worktree = \"fix\" + +[[steps]] +name = \"ship\" +catalogue = \"delivery\" +worktree = \"notes\" +", + ":18: delivery:", + ); + refuse_issue_branch( + "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[worktrees.notes] +kind = \"throwaway\" + +[[steps]] +name = \"implement\" +skill = \"code-review\" +worktree = \"notes\" +parallel = \"together\" + +[[steps]] +name = \"ship\" +catalogue = \"delivery\" +worktree = \"fix\" +parallel = \"together\" +", + ":20: delivery:", + ); + refuse_issue_branch( + "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[worktrees.notes] +kind = \"throwaway\" + +[[steps]] +name = \"notes\" +command = \"true\" +worktree = \"notes\" +", + ":5: delivery:", + ); + refuse_issue_branch( + "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[[steps]] +name = \"implement\" +skill = \"code-review\" +worktree = \"fix\" + +[[steps]] +name = \"ship\" +catalogue = \"delivery\" +worktree = \"fix\" + +[[steps]] +name = \"review\" +skill = \"code-review\" +worktree = \"fix\" +", + ":20: delivery:", + ); +} + +#[test] +fn an_issue_branch_process_ends_ready_for_review_and_removes_the_claim() { + let scenario = Scenario::new(); + write_process(&scenario, "ship", SHIP); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does_for_in_session( + 7, + 1, + r#" +gh issue view 7 --repo acme/widgets --json labels | grep -q in-progress +echo feature > feature.txt +git add feature.txt +git commit -q -m "Add feature" +printf 'implemented' > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + scenario.agent_does_for_in_session( + 7, + 2, + r#" +test -f feature.txt +printf 'reviewed' > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + + let result = scenario.run(&["process", "ship", "issue", "7"]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + assert_eq!(scenario.claude_calls().len(), 2, "{}", result.stderr); + let pr = &scenario.gh_state()["prs"][0]; + assert_eq!(pr["state"], "OPEN"); + assert_eq!(pr["isDraft"], false); + assert_eq!(pr["head"], "issue-7"); + assert_eq!(pr["base"], "main"); + assert!( + pr["body"].as_str().unwrap().contains("Closes #7"), + "{}", + pr["body"] + ); + assert_eq!(scenario.issue_labels(7), Vec::::new()); + assert!( + result + .stderr + .contains("labelling #7 in-progress, in place of ready-for-agent"), + "{}", + result.stderr + ); + assert!( + result.stderr.contains("removing in-progress from issue #7"), + "{}", + result.stderr + ); + assert_eq!( + scenario.origin_file("issue-7", "feature.txt").as_deref(), + Some("feature\n") + ); + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn a_step_that_fails_before_delivery_keeps_the_issue_branch_work() { + let scenario = Scenario::new(); + write_process(&scenario, "ship", SHIP); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does_for( + 7, + r#" +echo left > left.txt +git add left.txt +exit 1 +"#, + ); + + let result = scenario.run(&["process", "ship", "issue", "7"]); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(scenario.claude_calls().len(), 1, "{}", result.stderr); + assert!(scenario.gh_state()["prs"].as_array().unwrap().is_empty()); + assert!( + result.stderr.contains("Stopped at: Implementation"), + "{}", + result.stderr + ); + assert!( + result.stderr.contains("Preserved: remotely on issue-7"), + "{}", + result.stderr + ); + assert!( + result.stderr.contains( + "Recovery: thirdshift process ship issue https://github.com/acme/widgets/issues/7" + ), + "{}", + result.stderr + ); + assert!( + result.stderr.contains("Run from the Launch directory"), + "{}", + result.stderr + ); + assert!( + result.stderr.contains("Continuing reuses"), + "{}", + result.stderr + ); + assert_eq!( + scenario.origin_file("issue-7", "left.txt").as_deref(), + Some("left\n") + ); + assert_eq!(scenario.issue_labels(7), ["in-progress"]); + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn a_delivery_repair_uses_the_commands_harness_model_and_effort() { + let scenario = Scenario::new(); + write_process( + &scenario, + "ship", + "\ +version = 1 +description = \"Ship a fix\" + +[worktrees.fix] +kind = \"issue-branch\" + +[[steps]] +name = \"implement\" +skill = \"code-review\" +worktree = \"fix\" + +[[steps]] +name = \"ship\" +catalogue = \"delivery\" +worktree = \"fix\" +", + ); + scenario.user_config_is( + "[harness]\ndefault = \"claude\"\n[harness.claude]\nmodel = \"opus\"\neffort = \"low\"\n", + ); + scenario.issue_labelled(7, &["ready-for-agent"]); + let red = r#"[{"name": "test", "conclusion": "failure", "url": "https://ci.example/test"}]"#; + let green = r#"[{"name": "test", "conclusion": "success", "url": "https://ci.example/test"}]"#; + scenario.agent_does_for_in_session( + 7, + 1, + &format!( + "echo feature > feature.txt\n\ + git add feature.txt\n\ + git commit -q -m 'Add feature'\n\ + gh fake checks \"$(git rev-parse HEAD)\" '{red}'\n\ + printf 'implemented' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n" + ), + ); + scenario.agent_does_for_in_session( + 7, + 2, + &format!( + "echo fix > fix-1.txt\n\ + git add fix-1.txt\n\ + git commit -q -m 'Fix CI 1'\n\ + gh fake checks \"$(git rev-parse HEAD)\" '{green}'\n\ + printf 'fixed' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n" + ), + ); + + let result = scenario.run(&["process", "ship", "issue", "7"]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + assert!( + result.stderr.contains("CI red; starting Repair"), + "{}", + result.stderr + ); + let calls = scenario.claude_calls(); + assert_eq!(calls.len(), 2, "{}", result.stderr); + assert!( + calls[1]["prompt"] + .as_str() + .unwrap() + .starts_with("CI failed on pull request "), + "{}", + calls[1]["prompt"] + ); + assert_eq!(calls[1]["cwd"], calls[0]["cwd"]); + for call in &calls { + let args: Vec = call["argv"] + .as_array() + .unwrap() + .iter() + .map(|arg| arg.as_str().unwrap().to_string()) + .collect(); + let value = |flag: &str| { + args.iter() + .position(|arg| arg == flag) + .and_then(|at| args.get(at + 1)) + .map(String::as_str) + }; + assert_eq!(value("--model"), Some("opus"), "{args:?}"); + assert_eq!(value("--effort"), Some("low"), "{args:?}"); + } + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], false); + assert_eq!(scenario.origin_log("issue-7").unwrap()[0], "Fix CI 1"); + assert_eq!(scenario.issue_labels(7), Vec::::new()); +} + +#[test] +fn a_continuation_reuses_the_open_issue_branch() { + let scenario = Scenario::new(); + write_process(&scenario, "ship", SHIP); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + let url = scenario.github_has_pr("issue-7", "main", "OPEN"); + scenario.issue_labelled(7, &["in-progress"]); + scenario.agent_does_for_in_session( + 7, + 1, + r#" +echo more > more.txt +git add more.txt +git commit -q -m "Add more" +printf 'continued' > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + scenario.agent_does_for_in_session( + 7, + 2, + "printf 'reviewed' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n", + ); + + let result = scenario.run(&["process", "ship", "issue", "7"]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(result.stdout, format!("{url}\n")); + assert_eq!(scenario.gh_state()["prs"].as_array().unwrap().len(), 1); + assert_eq!(scenario.gh_state()["prs"][0]["head"], "issue-7"); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], false); + assert!(scenario.origin_log("issue-7-branch-2").is_none()); + assert_eq!(scenario.origin_log("issue-7").unwrap()[0], "Add more"); + assert_eq!(scenario.issue_labels(7), Vec::::new()); + scenario.assert_cleaned_up("issue-7"); +} From 2bb0b604f86fae8fcad75bc682785a3c35a592aa Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 26 Oct 2026 23:14:51 -0400 Subject: [PATCH 2/3] Tell Continuation Steps to build on the Issue branch's commits (#1388) --- GLOSSARY.md | 2 +- docs/guide.md | 7 +- site/architecture/index.html | 2 +- site/docs/index.html | 7 +- site/index.html | 2 +- src/delivery.rs | 249 +++++++++++++++++++---------------- src/processes.rs | 75 ++++++----- src/prompt.rs | 32 ++++- src/prompts_page.rs | 3 + src/recovery.rs | 18 +++ tests/process.rs | 35 +++++ 11 files changed, 274 insertions(+), 158 deletions(-) diff --git a/GLOSSARY.md b/GLOSSARY.md index f5f2e10c..844be537 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -116,7 +116,7 @@ One invocation of the factory with no **Issue URL**: a **Security audit** of the _Avoid_: Fencing (former name), Secure run (it reads as a run that is secure), audit run, security pass **Pass**: -An **Architect run**, a **Pickup run**, a **Security run**, or a **Custom process** started with no issue: a **Command** started with no **Issue URL**, which decides before any work whether it is skipped. A Custom process started with no issue is skipped while another Pass on the same repository is still running on the machine, its Spec run or Ticket run included, and it does not take an Architect run's Ready-issue or Architect-idea gates. A Custom process started with an issue is not a Pass. +An **Architect run**, a **Pickup run**, a **Security run**, or a **Custom process** started with no issue and no Issue-branch worktree: a **Command** started with no **Issue URL**, which decides before any work whether it is skipped. A Custom process started with no issue and no Issue-branch worktree is skipped while another Pass on the same repository is still running on the machine, its Spec run or Ticket run included, and it does not take an Architect run's Ready-issue or Architect-idea gates. A file that declares an Issue-branch worktree is refused when no issue is given, before any session, and is not a Pass. A Custom process started with an issue is not a Pass. _Avoid_: tick, cycle ### Architecture diff --git a/docs/guide.md b/docs/guide.md index 15caff69..cdcd55ce 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -17,7 +17,7 @@ A [Ticket run](#what-a-ticket-run-does), one issue in and one pull request out, - **[Pickup run](#pickup-runs)**, `thirdshift pickup`: takes the lowest-numbered **Ready issue** by default, the highest-numbered with `pickup.order = "highest-first"` or `highest-first` on the command, and runs it. - **[Security run](#security-runs)**, `thirdshift secure`: audits the **Base branch** and records each **Security finding** privately. It is report-only by default; with fixing allowed, it fixes one finding it reproduced. -Architect, Pickup and Security runs are **Passes**, as is a Custom process started with no issue: they take no Issue URL, so your operating system's scheduler can start them. A Custom process started with an issue is not a Pass. See [A Pickup run on a schedule](#a-pickup-run-on-a-schedule), [Architect runs every few minutes](#architect-runs-every-few-minutes) for Architect runs, and [Security runs on a schedule, with fixing allowed](#security-runs-on-a-schedule-with-fixing-allowed) for Security runs. +Architect, Pickup and Security runs are **Passes**, as is a Custom process started with no issue and no Issue-branch worktree: they take no Issue URL, so your operating system's scheduler can start them. A Custom process started with an issue is not a Pass. A file that declares an Issue-branch worktree is refused when no issue is given, before any session, rather than skipped. See [A Pickup run on a schedule](#a-pickup-run-on-a-schedule), [Architect runs every few minutes](#architect-runs-every-few-minutes) for Architect runs, and [Security runs on a schedule, with fixing allowed](#security-runs-on-a-schedule-with-fixing-allowed) for Security runs. Every session runs on the **Harness** you choose: Claude Code by default, or Codex, Antigravity CLI, Grok Build, Muse Code or OpenCode. See [Harness, Model and Effort](#harness-model-and-effort). @@ -939,7 +939,8 @@ A declared worktree's `kind` is `throwaway` or `issue-branch`. A throwaway worktree is detached at the Base branch. An Issue-branch worktree is the Issue branch a Ticket run would use for the issue the command names: `issue-`, then `issue--branch-` for k ≥ 2. A Continuation reuses -the existing branch, and an open pull request's base is the Base branch. A +the existing branch, and an open pull request's base is the Base branch. +Session Steps in that checkout are told to build on its commits. A Process has at most one Issue-branch worktree. The Steps that name it run in that checkout. Any other declared worktree stays throwaway. @@ -1740,7 +1741,7 @@ The rule doesn't ask whether anything will take the Ready issue. A `ready-for-ag A **Pass** is a command started with no Issue URL that decides before any work whether it is skipped. Only one Pass per repository runs at a time on a machine. A Pass started while another on the same repository is still running, the Spec run or Ticket run it dispatched included, is **skipped**: it prints `another Pass is already running on /` as a progress line on stderr, prints nothing on stdout, and exits `0`. Skipped is not a failure. A skipped Pass does nothing else: no launch pull, no worktree, no agent session, and no issue filed or labelled. This holds for every Pass, however it was started, so two never build one repository at once. -The repository is the one `origin` names, so two clones of one repository count as the same, and Passes on different repositories run at the same time. A Ticket run or a Spec run started on an **Issue URL** is never skipped this way, and never makes a Pass skip. A Custom process started with an issue is the same. A Custom process started with no issue is a Pass for this rule only: it does not take an Architect run's Ready-issue or Architect-idea gates. Its skip keeps a Command record whose outcome is `skipped`. +The repository is the one `origin` names, so two clones of one repository count as the same, and Passes on different repositories run at the same time. A Ticket run or a Spec run started on an **Issue URL** is never skipped this way, and never makes a Pass skip. A Custom process started with an issue is the same. A Custom process started with no issue, and whose file declares no Issue-branch worktree, is a Pass for this rule only: it does not take an Architect run's Ready-issue or Architect-idea gates. Its skip keeps a Command record whose outcome is `skipped`. A file that declares an Issue-branch worktree is refused when no issue is given, before any session, rather than skipped. thirdshift holds the rule with an operating-system lock on a file under `~/.thirdshift/architect-locks/`, shared by every kind of Pass, which the Pass's process takes without waiting, after its checks in step 1, and holds until it exits. The directory keeps its original name so older thirdshift versions share the lock too. The operating system releases the lock when that process ends for any reason, a crash, a kill or a reboot included, so there is never a stale lock to clear: the next Pass on the repository runs normally. The file itself stays, and means nothing on its own. A review worktree that a killed Architect run left behind is removed by the next one. diff --git a/site/architecture/index.html b/site/architecture/index.html index 93e44c6d..866b0f7f 100644 --- a/site/architecture/index.html +++ b/site/architecture/index.html @@ -478,7 +478,7 @@

Architecture

Figure 5 · thirdshift pickup, architect, secure

The Passes: the factory finds its own work

Before selecting a Ready issue, Pickup files one ordinary bug Ticket when the Base branch's latest push CI failed more than 30 minutes ago and no CI-red Ticket is open. Concurrent hosts reserve filing on origin. CI read failures warn and leave Pickup running; this check does not hold Self-merges.

-

Once a Pickup run, an Architect run or a Security run knows its repository, it replaces that kind's last-fired time under the repository's log folder, before launch checks or the local Pass lock. Repeated skips update the time without adding Activity log lines. thirdshift status, thirdshift cronjobs and their Console views read the same per-repository times; no reader starts work. A Custom process started with no issue shares the lock and does not record a last-fired time.

+

Once a Pickup run, an Architect run or a Security run knows its repository, it replaces that kind's last-fired time under the repository's log folder, before launch checks or the local Pass lock. Repeated skips update the time without adding Activity log lines. thirdshift status, thirdshift cronjobs and their Console views read the same per-repository times; no reader starts work. A Custom process started with no issue, and no Issue-branch worktree, shares the lock and does not record a last-fired time. A file that declares an Issue-branch worktree is refused when no issue is given, before the lock.

A Pass takes no Issue URL, so your operating system's scheduler can start it; thirdshift never schedules itself. Only one Pass runs on a repository at a time, and a Pass with nothing to do is skipped, not failed. Architect and Security runs wait while a Ready issue is open, so work you shaped goes first. A Custom process started with no issue is a Pass for that one-at-a-time rule and does not wait on a Ready issue or an Architect idea, unless the file declares an Issue-branch worktree: that is refused before any session, rather than skipped. One started with an issue is not a Pass. Your scheduler can start Architect runs every few minutes and Security runs on a schedule, with fixing allowed.

The three PassesThe operating system’s scheduler starts three kinds of Pass, each taking no Issue URL. A Pickup run sweeps closed issues, respects the Claim limit and runs the lowest-numbered Ready issue by default. An Architect run reviews the architecture and either publishes a plan, which it builds with a Ticket run or Spec run, or files an idea for the Day shift to triage. A Security run audits the Base branch, reproduces findings, records each privately and, when fixing is allowed, files a fix issue and runs it. Its target code may run only inside an OS-enforced sandbox, as the Security audit skill requires. diff --git a/site/docs/index.html b/site/docs/index.html index 0ae3530b..97f0565c 100644 --- a/site/docs/index.html +++ b/site/docs/index.html @@ -64,7 +64,7 @@
  • Pickup run, thirdshift pickup: takes the lowest-numbered Ready issue by default, the highest-numbered with pickup.order = "highest-first" or highest-first on the command, and runs it.
  • Security run, thirdshift secure: audits the Base branch and records each Security finding privately. It is report-only by default; with fixing allowed, it fixes one finding it reproduced.
    • -

    Architect, Pickup and Security runs are Passes, as is a Custom process started with no issue: they take no Issue URL, so your operating system's scheduler can start them. A Custom process started with an issue is not a Pass. See A Pickup run on a schedule, Architect runs every few minutes for Architect runs, and Security runs on a schedule, with fixing allowed for Security runs.

    +

    Architect, Pickup and Security runs are Passes, as is a Custom process started with no issue and no Issue-branch worktree: they take no Issue URL, so your operating system's scheduler can start them. A Custom process started with an issue is not a Pass. A file that declares an Issue-branch worktree is refused when no issue is given, before any session, rather than skipped. See A Pickup run on a schedule, Architect runs every few minutes for Architect runs, and Security runs on a schedule, with fixing allowed for Security runs.

    Every session runs on the Harness you choose: Claude Code by default, or Codex, Antigravity CLI, Grok Build, Muse Code or OpenCode. See Harness, Model and Effort.

    What a Ticket run does

     @@ -852,7 +852,8 @@ worktree is detached at the Base branch. An Issue-branch worktree is the Issue branch a Ticket run would use for the issue the command names: issue-<n>, then issue-<n>-branch-<k> for k ≥ 2. A Continuation reuses -the existing branch, and an open pull request's base is the Base branch. A +the existing branch, and an open pull request's base is the Base branch. +Session Steps in that checkout are told to build on its commits. A Process has at most one Issue-branch worktree. The Steps that name it run in that checkout. Any other declared worktree stays throwaway.

    catalogue = "delivery" publishes that Issue branch. It is Delivery with no @@ -1510,7 +1511,7 @@

    One at a time

    A Pass is a command started with no Issue URL that decides before any work whether it is skipped. Only one Pass per repository runs at a time on a machine. A Pass started while another on the same repository is still running, the Spec run or Ticket run it dispatched included, is skipped: it prints another Pass is already running on <owner>/<repo> as a progress line on stderr, prints nothing on stdout, and exits 0. Skipped is not a failure. A skipped Pass does nothing else: no launch pull, no worktree, no agent session, and no issue filed or labelled. This holds for every Pass, however it was started, so two never build one repository at once.

    -

    The repository is the one origin names, so two clones of one repository count as the same, and Passes on different repositories run at the same time. A Ticket run or a Spec run started on an Issue URL is never skipped this way, and never makes a Pass skip. A Custom process started with an issue is the same. A Custom process started with no issue is a Pass for this rule only: it does not take an Architect run's Ready-issue or Architect-idea gates. Its skip keeps a Command record whose outcome is skipped.

    +

    The repository is the one origin names, so two clones of one repository count as the same, and Passes on different repositories run at the same time. A Ticket run or a Spec run started on an Issue URL is never skipped this way, and never makes a Pass skip. A Custom process started with an issue is the same. A Custom process started with no issue, and whose file declares no Issue-branch worktree, is a Pass for this rule only: it does not take an Architect run's Ready-issue or Architect-idea gates. Its skip keeps a Command record whose outcome is skipped. A file that declares an Issue-branch worktree is refused when no issue is given, before any session, rather than skipped.

    thirdshift holds the rule with an operating-system lock on a file under ~/.thirdshift/architect-locks/, shared by every kind of Pass, which the Pass's process takes without waiting, after its checks in step 1, and holds until it exits. The directory keeps its original name so older thirdshift versions share the lock too. The operating system releases the lock when that process ends for any reason, a crash, a kill or a reboot included, so there is never a stale lock to clear: the next Pass on the repository runs normally. The file itself stays, and means nothing on its own. A review worktree that a killed Architect run left behind is removed by the next one.

    A skipped Architect run sends no Run notification, even when asked, and neither does a skipped Pickup run (sends none). Both make the notification's checks before the lock is tried all the same, so a broken setup fails the pass with exit 1 rather than being skipped.

    diff --git a/site/index.html b/site/index.html index 1d1364af..c7043c8a 100644 --- a/site/index.html +++ b/site/index.html @@ -355,7 +355,7 @@

    Six workflows

    An Architect run looks for architecture improvements and builds its top Strong plan. Without a Strong recommendation, it leaves an Architect idea for the Day shift. Use --plan-only to publish a plan and stop.

    A Pickup run takes the lowest-numbered Ready issue by default, the highest-numbered with pickup.order = "highest-first" or highest-first, after it has settled and while the repository is below its Claim limit. It also files one ordinary bug Ticket when the Base branch's latest push CI has stayed red for more than 30 minutes. Failed work stays with the Day shift until you hand it back by applying ready-for-agent again.

    A Security run audits the Base branch and records findings privately. It is report-only by default. With security-fix, it can fix a reproduced finding through a Ticket run or Spec run. With security-close, it can close a finding its fix merged, once its proof-of-concept test passes against the merged Base branch. An optional Security review checks each Ticket run's change, including Tickets and the Spec PR, and holds Self-merge if introduced findings remain unaddressed or the review is incomplete. Its Harness is checked before work; an unavailable review Harness falls back to the default Harness, with one fallback also allowed for a Harness error before its first turn.

    -

    Start passes with thirdshift architect base main, thirdshift pickup base main or thirdshift secure base main. A Custom process started with no issue follows the same one-at-a-time rule and does not wait on a Ready issue or an Architect idea. Your operating system schedules them. thirdshift status, thirdshift cronjobs and the Console show when each repository's Pickup, Architect and Security Pass last fired, including repeated skips. Architect runs every few minutes and Security runs on a schedule, with fixing allowed. Both yield to work the Day shift shaped, and pause when a decision needs you.

    +

    Start passes with thirdshift architect base main, thirdshift pickup base main or thirdshift secure base main. A Custom process started with no issue, and no Issue-branch worktree, follows the same one-at-a-time rule and does not wait on a Ready issue or an Architect idea. A file that declares an Issue-branch worktree is refused when no issue is given, before any session. Your operating system schedules them. thirdshift status, thirdshift cronjobs and the Console show when each repository's Pickup, Architect and Security Pass last fired, including repeated skips. Architect runs every few minutes and Security runs on a schedule, with fixing allowed. Both yield to work the Day shift shaped, and pause when a decision needs you.

    The Console opens on this machine's Inbox: Commands grouped by what needs you, with their stages and logs. It reads ready rows' pull requests and Needs you rows' issues and pull requests from GitHub in the background once a minute; changed answers update open pages automatically. thirdshift status uses the same kept answer. Failed reads keep the last facts; after five minutes, both show when GitHub last answered and why the latest read failed. With fewer than 500 GraphQL points or REST requests left, both wait for the quota to reset, leaving the rest to the factory. An ended Needs you row leaves when GitHub shows its issue closed or its pull request merged; a Triage row also leaves when needs-triage is removed. A Finding leaves with “findings triaged” once all its private records are triaged on GitHub. A draft advisory without severity, or an open security-finding issue labelled needs-triage, still waits; a finding closed by its merged fix does not. Live Silent and Slow rows wait until their Commands end. A signed-in account can Stop or Requeue a live Command, just as from the CLI. On an Inbox row or in Command details, Dismiss gives a row in Needs you or Ready for review a Dismissal in one click; Undo takes it back. Command details shows each CLI form: thirdshift dismiss <command> or thirdshift dismiss <command> --undo. A dismissed row returns for a new kind of problem or when it ends ready for review; each kind counts once for the whole row.

    Above its groups, the Inbox counts the runs active on this machine. The Console's Resources page, also available with thirdshift resources, shows host memory, CPU load and the User config limits that can delay new sessions.

    Experiments compare a Candidate with a Baseline on private Eval suites. thirdshift experiment <candidate> builds both Arms and runs interleaved Trials with two workers, a fixed Bar and resume after interruption or a plan-limit warning. Trials and Graders run in bwrap sandboxes. Code Graders score mechanical checks offline from report packets and the prepared repository, then fresh model Graders score written criteria from blind packets with provider-only access. Graders cannot read raw Trial logs, Arm mappings or the Experiment’s other files. Results show the Verdict, wrong findings, borderline marks, paired intervals, each Arm's minutes, tokens by Model, reported cost and Void causes. Trials and Graders join the Command's Trace. Void Trials retry fresh up to twice; an exhausted task drops out of both Arms. More than a tenth dropped makes the Verdict inconclusive.

    diff --git a/src/delivery.rs b/src/delivery.rs index 241db8a1..67778e76 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -130,74 +130,60 @@ impl Delivery<'_> { checklist: Option<&str>, ) -> Result { let tracker = Tracker::new(opening.session.stage(), CommandLog); - let route = Route { - security_review: self.security_review, - issue: self.issue, - base: self.base, - branch: worktree.branch(), - goal: self.goal, - harness: &self.child_asks.harness, - }; - pull_request.begin_delivery(); - let (issue, started, path) = (self.issue, self.started, worktree.path()); - let before_each = move || { - no_untrusted_edit_since(issue, started)?; - hand_over_issue(issue, path) - }; - let (delivered, log) = Sessions::checked_within( - self.logs, - worktree.path(), - &self.child_asks.harness, - self.child_asks.security_review_harness.as_ref(), - self.child_asks.security_review_default.as_ref(), - &before_each, - |sessions| { + let logs = self.logs; + let issue = self.issue; + let started = self.started; + let base = self.base; + let goal = self.goal; + let security_review = self.security_review; + let child_asks = self.child_asks; + let base_fix = self.base_fix; + run_checked( + logs, + issue, + started, + &child_asks.harness, + child_asks.security_review_harness.as_ref(), + child_asks.security_review_default.as_ref(), + base, + worktree, + pull_request, + &tracker, + |resolution, log| Reached { + resolution, + log, + ticket_lines: Vec::new(), + }, + |merged, log| Reached { + resolution: Resolution::PullRequest { + url: merged.pr.url.clone(), + goal: Goal::Merged, + }, + log, + ticket_lines: Vec::new(), + }, + |tracker, worktree, pull_request, sessions| { + let route = Route { + security_review, + issue, + base, + branch: worktree.branch(), + goal, + harness: &child_asks.harness, + }; let mut outside = InWorktree { - tracker: &tracker, - issue: self.issue, - worktree: &worktree, - base: self.base, - base_fix: self.base_fix, - child_asks: self.child_asks, + tracker, + issue, + worktree, + base, + base_fix, + child_asks, sessions, pull_request, }; route.steps(&mut outside, &opening, checklist) }, - ); - let result = match delivered { - Ok(resolution) => Ok(Reached { - resolution, - log, - ticket_lines: Vec::new(), - }), - Err(error) => { - if let Some(merged) = error.downcast_ref::() - && worktree.fully_landed_at(&merged.head).unwrap_or(false) - { - progress::step("the pull request was merged while Delivery was running"); - Ok(Reached { - resolution: Resolution::PullRequest { - url: merged.pr.url.clone(), - goal: Goal::Merged, - }, - log, - ticket_lines: Vec::new(), - }) - } else { - let stopped_at = stopped_at(&tracker, &error); - let path = worktree.path().to_path_buf(); - let mut outside = OnFailure { - base: self.base, - worktree, - pull_request, - }; - Err(fail(&mut outside, stopped_at, path, log, error)) - } - } - }; - crate::logs::unaddressed_findings(pull_request.unaddressed_findings()); - result + ) } /// Publish the Issue branch already produced by a Custom process and @@ -214,57 +200,98 @@ impl Delivery<'_> { pull_request: &mut PullRequest, ) -> Result { let tracker = Tracker::new(Stage::Push, CommandLog); - pull_request.begin_delivery(); - let (issue, started, path) = (self.issue, self.started, worktree.path().to_path_buf()); - let before_each = move || { - no_untrusted_edit_since(issue, started)?; - hand_over_issue(issue, &path) - }; - let (delivered, log) = Sessions::checked_within( - self.logs, - worktree.path(), - &self.child_asks.harness, - self.child_asks.security_review_harness.as_ref(), - self.child_asks.security_review_default.as_ref(), - &before_each, - |sessions| { + let logs = self.logs; + let issue = self.issue; + let started = self.started; + let base = self.base; + let child_asks = self.child_asks; + let base_fix = self.base_fix; + run_checked( + logs, + issue, + started, + &child_asks.harness, + child_asks.security_review_harness.as_ref(), + child_asks.security_review_default.as_ref(), + base, + worktree, + pull_request, + &tracker, + |url, _log| url, + |merged, _log| merged.pr.url.clone(), + |tracker, worktree, pull_request, sessions| { publish_steps( - &tracker, - self.issue, - self.base, - self.child_asks, - self.base_fix, - &worktree, + tracker, + issue, + base, + child_asks, + base_fix, + worktree, pull_request, sessions, ) }, - ); - let result = match delivered { - Ok(url) => Ok(url), - Err(error) => { - if let Some(merged) = error.downcast_ref::() - && worktree.fully_landed_at(&merged.head).unwrap_or(false) - { - progress::step("the pull request was merged while Delivery was running"); - Ok(merged.pr.url.clone()) - } else { - let stopped_at = stopped_at(&tracker, &error); - let path = worktree.path().to_path_buf(); - let mut outside = OnFailure { - base: self.base, - worktree, - pull_request, - }; - Err(fail(&mut outside, stopped_at, path, log, error)) - } - } - }; - crate::logs::unaddressed_findings(pull_request.unaddressed_findings()); - result + ) } } +/// The shared Delivery envelope: check for an untrusted edit before each +/// session, run `steps`, and on failure either accept an already-merged pull +/// request whose head includes the local work or take the Failed run path. +fn run_checked( + logs: &Logs, + issue: &IssueUrl, + started: DateTime, + harness: &Choice, + security_review: Option<&Choice>, + security_review_default: Option<&Choice>, + base: &str, + worktree: Worktree, + pull_request: &mut PullRequest, + tracker: &Tracker, + on_ok: impl FnOnce(T, Option) -> S, + on_merged: impl FnOnce(&AlreadyMerged, Option) -> S, + steps: impl FnOnce(&Tracker, &Worktree, &mut PullRequest, &Sessions) -> Result, +) -> Result { + pull_request.begin_delivery(); + let path = worktree.path().to_path_buf(); + let before_each = move || { + no_untrusted_edit_since(issue, started)?; + hand_over_issue(issue, &path) + }; + let (delivered, log) = Sessions::checked_within( + logs, + worktree.path(), + harness, + security_review, + security_review_default, + &before_each, + |sessions| steps(tracker, &worktree, pull_request, sessions), + ); + let result = match delivered { + Ok(value) => Ok(on_ok(value, log)), + Err(error) => { + if let Some(merged) = error.downcast_ref::() + && worktree.fully_landed_at(&merged.head).unwrap_or(false) + { + progress::step("the pull request was merged while Delivery was running"); + Ok(on_merged(merged, log)) + } else { + let stopped_at = stopped_at(&tracker, &error); + let path = worktree.path().to_path_buf(); + let mut outside = OnFailure { + base, + worktree, + pull_request, + }; + Err(fail(&mut outside, stopped_at, path, log, error)) + } + } + }; + crate::logs::unaddressed_findings(pull_request.unaddressed_findings()); + result +} + /// [`Delivery::publish`]'s steps: push, open a draft when none exists, write /// what it was built with, mark the pull request ready, and take it to ready /// for review through the Repair loop. @@ -527,16 +554,8 @@ fn fail( let preservation = if keep_ready { Preservation::Remote } else { - match outside.preserve_failed_run(&reason) { - Ok(Preserved::Pushed) => Preservation::Remote, - Ok(Preserved::NoChanges) => Preservation::None, - Err(problem) => { - outside.step(format!( - "could not push the failed ticket run's work, so it may exist only locally: {problem:#}" - )); - Preservation::Local - } - } + let preserved = outside.preserve_failed_run(&reason); + recovery::preservation_of(preserved, |line| outside.step(line)) }; let pr_url = match outside.finish_pr(keep_ready) { Ok(pr_url) => pr_url, diff --git a/src/processes.rs b/src/processes.rs index 0e019af0..59700096 100644 --- a/src/processes.rs +++ b/src/processes.rs @@ -42,11 +42,11 @@ use crate::logs::{self, Begin, Work}; use crate::notification::{About, NotificationAsk, RunNotification}; use crate::process::{self, Control, Interruption}; use crate::pull_request::PullRequest; -use crate::recovery::{self, Preservation, Recovery}; +use crate::recovery::{self, Recovery}; use crate::run::Goal; use crate::run_ending::{Account, Cause}; use crate::skill; -use crate::worktree::{Preserved, SkillWorktree, Worktree}; +use crate::worktree::{SkillWorktree, Worktree}; use crate::{delivery, interrupt, launch, progress, prompt, session, skills, trust}; /// The issue URL when the Process has an issue; unset otherwise. @@ -174,6 +174,8 @@ struct StepRun<'a> { choice: &'a Choice, issue: Option<&'a IssueUrl>, started: DateTime, + /// Branch and Base branch when this Step continues an Issue branch. + continuation: Option<(String, String)>, } struct WrittenGuard; @@ -510,10 +512,12 @@ fn run_issue_branch( process: name, }; let result = execute_steps(name, &prepared, Some(&mut issue_run)); + let context = continuation(&issue_run); + let kept_on_origin = issue_run.on_origin; if let Err(error) = &result && let Some(checkout) = issue_run.checkout.as_mut() { - preserve_step_failure(checkout, name, &issue, &base, &branch, on_origin, error); + preserve_step_failure(checkout, context, kept_on_origin, error); } result }; @@ -552,6 +556,10 @@ fn execute_steps( .with_context(|| format!("could not create {}", results.display()))?; let mut earlier = Vec::new(); let mut last = String::new(); + let continuing = issue_run.as_ref().and_then(|run| { + run.on_origin + .then(|| (run.branch.to_string(), run.base.to_string())) + }); for range in group_ranges(&prepared.steps) { let group = &prepared.steps[range.clone()]; if group.len() == 1 && matches!(group[0].kind, StepKind::Delivery) { @@ -581,6 +589,7 @@ fn execute_steps( &results, range.start == 0, checkout, + step_continuation(prepared, &group[0], continuing.as_ref()), ); vec![(started.elapsed(), outcome)] } else { @@ -592,6 +601,7 @@ fn execute_steps( &results, range.start, checkout, + continuing.as_ref(), ) }; let mut group_error = None; @@ -643,6 +653,7 @@ fn run_parallel( results: &Path, group_start: usize, checkout: Option<&Worktree>, + continuing: Option<&(String, String)>, ) -> Vec<(Duration, Result)> { thread::scope(|scope| { let mut handles = Vec::with_capacity(group.len()); @@ -655,6 +666,7 @@ fn run_parallel( let first = group_start + offset == 0; let issue = prepared.issue.clone(); let started_at = prepared.started; + let continuation = step_continuation(prepared, step, continuing); handles.push(scope.spawn(move || { let started = Instant::now(); let run = StepRun { @@ -662,6 +674,7 @@ fn run_parallel( choice: &choice, issue: issue.as_ref(), started: started_at, + continuation, }; let outcome = run_step_in(&run, &cwd, step, &earlier, &results, first); (started.elapsed(), outcome) @@ -717,12 +730,14 @@ fn run_step( results: &Path, first: bool, checkout: Option<&Worktree>, + continuation: Option<(String, String)>, ) -> Result { let run = StepRun { process, choice: &prepared.choice, issue: prepared.issue.as_ref(), started: prepared.started, + continuation, }; run_step_in( &run, @@ -740,14 +755,28 @@ fn step_dir<'a>( checkout: Option<&'a Worktree>, ) -> &'a Path { if let Some(checkout) = checkout - && step.worktree.is_some() - && prepared.issue_worktree.as_deref() == step.worktree.as_deref() + && names_issue_worktree(step.worktree.as_deref(), prepared.issue_worktree.as_deref()) { return checkout.path(); } worktree_path(&prepared.worktrees, step.worktree.as_deref()) } +/// Branch and Base branch for a Step whose checkout continues the Issue branch. +fn step_continuation( + prepared: &Prepared, + step: &ResolvedStep, + continuing: Option<&(String, String)>, +) -> Option<(String, String)> { + let (branch, base) = continuing?; + names_issue_worktree(step.worktree.as_deref(), prepared.issue_worktree.as_deref()) + .then(|| (branch.clone(), base.clone())) +} + +fn names_issue_worktree(step: Option<&str>, issue: Option<&str>) -> bool { + matches!((step, issue), (Some(step), Some(issue)) if step == issue) +} + fn run_delivery( prepared: &Prepared, step: &ResolvedStep, @@ -812,35 +841,16 @@ fn continuation(issue_run: &IssueRun<'_>) -> recovery::Continuation { fn preserve_step_failure( checkout: &mut Worktree, - process: &str, - issue: &IssueUrl, - base: &str, - branch: &str, + context: recovery::Continuation, on_origin: bool, error: &anyhow::Error, ) { let reason = Cause::of(error).first_line().to_string(); - let preservation = match checkout.preserve_failed_run(base, &reason, &mut || Ok(())) { - Ok(Preserved::Pushed) => Preservation::Remote, - Ok(Preserved::NoChanges) => Preservation::None, - Err(problem) => { - progress::step(format!( - "could not push the failed ticket run's work, so it may exist only locally: {problem:#}" - )); - Preservation::Local - } - }; + let preserved = checkout.preserve_failed_run(&context.base, &reason, &mut || Ok(())); + let preservation = recovery::preservation_of(preserved, |line| progress::step(line)); let mut recovery = Recovery::new(recovery::Stage::Implementation, preservation); recovery.record_worktree(checkout.path().to_path_buf()); - recovery.add_command_context( - recovery::Continuation { - command: format!("thirdshift process {process} issue {}", issue.url), - launch: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), - base: base.to_string(), - branch: branch.to_string(), - }, - on_origin, - ); + recovery.add_command_context(context, on_origin); for line in recovery.lines() { progress::step(line); } @@ -870,6 +880,9 @@ fn run_step_in( earlier, first, run.issue, + run.continuation + .as_ref() + .map(|(branch, base)| prompt::ProcessContinuation { branch, base }), ); match run.issue { Some(issue) => { @@ -1173,8 +1186,10 @@ fn resolve_dir<'a>( issue_checkout: Option<&'a Path>, ) -> &'a Path { if let Some(checkout) = issue_checkout - && step.worktree.as_ref().map(|(name, _)| name.as_str()) == issue_worktree - && issue_worktree.is_some() + && names_issue_worktree( + step.worktree.as_ref().map(|(name, _)| name.as_str()), + issue_worktree, + ) { return checkout; } diff --git a/src/prompt.rs b/src/prompt.rs index c948dfb1..32c546c8 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -109,11 +109,26 @@ pub fn skill(name: &str, path: &std::path::Path, issue: Option<&IssueUrl>) -> St ) } +/// Branch and Base branch of a Step that continues an existing Issue branch. +pub struct ProcessContinuation<'a> { + pub branch: &'a str, + pub base: &'a str, +} + +/// The sentence a Continuation uses so the session builds on existing commits. +fn continuing_work(branch: &str, base: &str) -> String { + format!( + "You are continuing work on branch {branch}, which already has commits (see git log {base}..HEAD). Build on them; don't start over." + ) +} + /// One Step of a Custom process. `earlier` is each finished Step's name and /// the path of its final message, outside the worktree. `first` is whether /// this is the Process's first Step. A later Step with no finished result /// does not claim to be the first. `issue`, when the Process has one, is -/// named for the session to work from its file. +/// named for the session to work from its file. `continuation`, when the +/// Step's checkout is an existing Issue branch, tells the session to build +/// on that branch's commits. pub fn process_step( process: &str, step: &str, @@ -122,9 +137,14 @@ pub fn process_step( earlier: &[(String, std::path::PathBuf)], first: bool, issue: Option<&IssueUrl>, + continuation: Option>, ) -> String { + let continuing = continuation + .as_ref() + .map(|note| format!("{}\n", continuing_work(note.branch, note.base))) + .unwrap_or_default(); let mut earlier_text = if earlier.is_empty() { - if first { + if first && continuation.is_none() { "This is the first Step. No earlier Step has a result.\n".to_string() } else { "No earlier Step has a result.\n".to_string() @@ -148,7 +168,7 @@ pub fn process_step( }); format!( "{preamble}This is Step {step} of the Custom process {process}.\n\ - {issue_text}{earlier_text}{SOURCE_EDITS}{HEADLESS}", + {issue_text}{continuing}{earlier_text}{SOURCE_EDITS}{HEADLESS}", preamble = skill_preamble(skill_name, skill_path), ) } @@ -249,7 +269,7 @@ pub fn continuation(issue: &IssueUrl, base: &str, branch: &str, pr_url: Option<& format!( "/thirdshift-implement {url}\n\ \n\ - You are continuing work on branch {branch}, which already has commits (see git log {base}..HEAD). Build on them; don't start over.\n\ + {continuing}\n\ \n\ The base branch is {base}. Review with the `thirdshift-code-review` skill using {base} as the fixed point.\n\ \n\ @@ -268,6 +288,7 @@ pub fn continuation(issue: &IssueUrl, base: &str, branch: &str, pr_url: Option<& {PR_LIFECYCLE}\n\ {HEADLESS}", url = issue.url, + continuing = continuing_work(branch, base), issue_file_instruction = issue_file_instruction(issue), number = issue.number, address_findings = review.address_findings(), @@ -698,6 +719,7 @@ mod tests { &[], true, Some(&issue), + None, ), ] { for instruction in [ @@ -721,6 +743,7 @@ mod tests { &[], true, Some(&issue), + None, ); assert!( on_issue.contains("Run this Step on issue https://github.com/acme/widgets/issues/7."), @@ -734,6 +757,7 @@ mod tests { &[], true, None, + None, ); assert!(!without.contains(".thirdshift-issue.md"), "{without}"); assert!(!without.contains("Run this Step on issue"), "{without}"); diff --git a/src/prompts_page.rs b/src/prompts_page.rs index 1c17ab1b..cb1c993a 100644 --- a/src/prompts_page.rs +++ b/src/prompts_page.rs @@ -334,6 +334,7 @@ fn prompts() -> Vec { &[], true, None, + None, ), }, Prompt { @@ -352,6 +353,7 @@ fn prompts() -> Vec { )], false, None, + None, ), }, Prompt { @@ -367,6 +369,7 @@ fn prompts() -> Vec { &[], true, Some(&issue), + None, ), }, ]; diff --git a/src/recovery.rs b/src/recovery.rs index f66a9df1..9b760f44 100644 --- a/src/recovery.rs +++ b/src/recovery.rs @@ -57,6 +57,24 @@ pub enum Preservation { None, } +/// Where a preserve attempt left the work. A push that fails is local, and +/// `report` receives the failed-ticket-run line. +pub fn preservation_of( + result: Result, + report: impl FnOnce(String), +) -> Preservation { + match result { + Ok(crate::worktree::Preserved::Pushed) => Preservation::Remote, + Ok(crate::worktree::Preserved::NoChanges) => Preservation::None, + Err(problem) => { + report(format!( + "could not push the failed ticket run's work, so it may exist only locally: {problem:#}" + )); + Preservation::Local + } + } +} + /// Public command context, filled after Base branch and effective asks settle. #[derive(Debug, PartialEq, Eq)] pub struct Continuation { diff --git a/tests/process.rs b/tests/process.rs index befc2aec..229953b7 100644 --- a/tests/process.rs +++ b/tests/process.rs @@ -3772,3 +3772,38 @@ printf 'continued' > "$FAKE_CLAUDE_FINAL_MESSAGE" assert_eq!(scenario.issue_labels(7), Vec::::new()); scenario.assert_cleaned_up("issue-7"); } + +#[test] +fn a_continuation_prompt_differs_from_a_fresh_start() { + let scenario = Scenario::new(); + write_process(&scenario, "ship", SHIP); + scenario.origin_has_branch("issue-7", "main", &["Earlier work"]); + let _url = scenario.github_has_pr("issue-7", "main", "OPEN"); + scenario.issue_labelled(7, &["in-progress"]); + scenario.agent_does_for_in_session( + 7, + 1, + "printf 'continued' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n", + ); + scenario.agent_does_for_in_session( + 7, + 2, + "printf 'reviewed' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n", + ); + + let result = scenario.run(&["process", "ship", "issue", "7"]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + let calls = scenario.claude_calls(); + let prompt = calls[0]["prompt"].as_str().unwrap(); + assert!( + prompt.contains("already has commits") + || prompt.contains("Build on them") + || prompt.contains("don't start over"), + "continuation prompt should differ from a fresh start: {prompt}" + ); + assert!( + !prompt.contains("This is the first Step. No earlier Step has a result."), + "continuation prompt still denies earlier work: {prompt}" + ); +} From 0fd5c0dc097821862edecb2a9df50ff09cf6560b Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 27 Oct 2026 00:35:30 -0400 Subject: [PATCH 3/3] Satisfy clippy on the Issue-branch Process (#1388) The test job rejected needless borrows, a redundant closure, and functions past clippy's argument limit. Delivery's envelope and publish steps, a Process's worktree source, and a Step prompt now carry those arguments in the types that already name them. --- src/delivery.rs | 122 ++++++++++++++++++++++++++------------------ src/processes.rs | 110 ++++++++++++++++++++++++--------------- src/prompt.rs | 46 ++++++++++------- src/prompts_page.rs | 24 +++++---- 4 files changed, 185 insertions(+), 117 deletions(-) diff --git a/src/delivery.rs b/src/delivery.rs index 67778e76..d4e896bc 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -139,13 +139,15 @@ impl Delivery<'_> { let child_asks = self.child_asks; let base_fix = self.base_fix; run_checked( - logs, - issue, - started, - &child_asks.harness, - child_asks.security_review_harness.as_ref(), - child_asks.security_review_default.as_ref(), - base, + CheckedRun { + logs, + issue, + started, + harness: &child_asks.harness, + security_review: child_asks.security_review_harness.as_ref(), + security_review_default: child_asks.security_review_default.as_ref(), + base, + }, worktree, pull_request, &tracker, @@ -207,45 +209,54 @@ impl Delivery<'_> { let child_asks = self.child_asks; let base_fix = self.base_fix; run_checked( - logs, - issue, - started, - &child_asks.harness, - child_asks.security_review_harness.as_ref(), - child_asks.security_review_default.as_ref(), - base, + CheckedRun { + logs, + issue, + started, + harness: &child_asks.harness, + security_review: child_asks.security_review_harness.as_ref(), + security_review_default: child_asks.security_review_default.as_ref(), + base, + }, worktree, pull_request, &tracker, |url, _log| url, |merged, _log| merged.pr.url.clone(), |tracker, worktree, pull_request, sessions| { - publish_steps( + publish_steps(&mut InWorktree { tracker, issue, + worktree, base, - child_asks, base_fix, - worktree, - pull_request, + child_asks, sessions, - ) + pull_request, + }) }, ) } } +/// What the shared Delivery envelope checks before each session and names +/// when a failure preserves the work: the issue, when the run began, where +/// its sessions are logged, the harnesses they run on, and the Base branch. +struct CheckedRun<'a> { + logs: &'a Logs, + issue: &'a IssueUrl, + started: DateTime, + harness: &'a Choice, + security_review: Option<&'a Choice>, + security_review_default: Option<&'a Choice>, + base: &'a str, +} + /// The shared Delivery envelope: check for an untrusted edit before each /// session, run `steps`, and on failure either accept an already-merged pull /// request whose head includes the local work or take the Failed run path. fn run_checked( - logs: &Logs, - issue: &IssueUrl, - started: DateTime, - harness: &Choice, - security_review: Option<&Choice>, - security_review_default: Option<&Choice>, - base: &str, + checked: CheckedRun<'_>, worktree: Worktree, pull_request: &mut PullRequest, tracker: &Tracker, @@ -253,6 +264,15 @@ fn run_checked( on_merged: impl FnOnce(&AlreadyMerged, Option) -> S, steps: impl FnOnce(&Tracker, &Worktree, &mut PullRequest, &Sessions) -> Result, ) -> Result { + let CheckedRun { + logs, + issue, + started, + harness, + security_review, + security_review_default, + base, + } = checked; pull_request.begin_delivery(); let path = worktree.path().to_path_buf(); let before_each = move || { @@ -277,7 +297,7 @@ fn run_checked( progress::step("the pull request was merged while Delivery was running"); Ok(on_merged(merged, log)) } else { - let stopped_at = stopped_at(&tracker, &error); + let stopped_at = stopped_at(tracker, &error); let path = worktree.path().to_path_buf(); let mut outside = OnFailure { base, @@ -295,43 +315,45 @@ fn run_checked( /// [`Delivery::publish`]'s steps: push, open a draft when none exists, write /// what it was built with, mark the pull request ready, and take it to ready /// for review through the Repair loop. -fn publish_steps<'a>( - tracker: &Tracker, - issue: &'a IssueUrl, - base: &'a str, - child_asks: &'a ChildAsks, - base_fix: &'a mut BaseFix, - worktree: &'a Worktree, - pull_request: &'a mut PullRequest, - sessions: &'a Sessions<'a>, -) -> Result { - tracker.enter(Stage::Push); - pull_request.ensure_actionable(false)?; - worktree.push_if_actionable(&mut || pull_request.ensure_actionable(false))?; - let title = GitHub::new().issue_title(issue)?; - let body = format!("Closes #{}", issue.number); - pull_request.open_if_absent(&title, &body)?; - if let Err(error) = pull_request.write_built_with(&child_asks.harness) { +fn publish_steps(outside: &mut InWorktree<'_>) -> Result { + outside.tracker.enter(Stage::Push); + outside.pull_request.ensure_actionable(false)?; + outside + .worktree + .push_if_actionable(&mut || outside.pull_request.ensure_actionable(false))?; + let title = GitHub::new().issue_title(outside.issue)?; + let body = format!("Closes #{}", outside.issue.number); + outside.pull_request.open_if_absent(&title, &body)?; + if let Err(error) = outside + .pull_request + .write_built_with(&outside.child_asks.harness) + { progress::warn( &error, format_args!("could not write the Built-with line in the pull request's body"), ); } - tracker.enter(Stage::PullRequestReadiness); - let pr = pull_request.mark_ready(None)?; - let mut outside = RunOutside { + outside.tracker.enter(Stage::PullRequestReadiness); + let tracker = outside.tracker; + let issue = outside.issue; + let worktree = outside.worktree; + let base = outside.base; + let child_asks = outside.child_asks; + let sessions = outside.sessions; + let pr = outside.pull_request.mark_ready(None)?; + let mut repair = RunOutside { tracker, issue, worktree, base, pr_url: &pr.url, - base_fix, + base_fix: outside.base_fix, child_asks, sessions, - pull_request, + pull_request: outside.pull_request, ci_watch: ci::Watch::new(issue), }; - repair_loop::take_to_goal(&mut outside, base, Goal::ReadyForReview)?; + repair_loop::take_to_goal(&mut repair, base, Goal::ReadyForReview)?; Ok(pr.url) } diff --git a/src/processes.rs b/src/processes.rs index 59700096..96319cb7 100644 --- a/src/processes.rs +++ b/src/processes.rs @@ -246,9 +246,11 @@ fn check(name: &str, issue: Option) -> Result { file, issue, Utc::now(), - &launch, - base.origin_commit(), - None, + WorktreeSource { + git: &launch, + commit: base.origin_commit(), + issue_checkout: None, + }, )?; Ok(plan(name, &prepared)) } @@ -323,9 +325,11 @@ fn run(name: &str, issue: Option, email: Option) -> file, None, started, - &launch, - base.origin_commit(), - None, + WorktreeSource { + git: &launch, + commit: base.origin_commit(), + issue_checkout: None, + }, ) { Ok(prepared) => prepared, Err(error) => return Err(failed_before_steps(notification, error)), @@ -351,9 +355,11 @@ fn run(name: &str, issue: Option, email: Option) -> file, Some(issue), started, - &launch, - base.origin_commit(), - None, + WorktreeSource { + git: &launch, + commit: base.origin_commit(), + issue_checkout: None, + }, ) { Ok(prepared) => prepared, Err(error) => return Err(failed_before_steps(notification, error)), @@ -417,8 +423,8 @@ fn finish_run( let result = execute_steps(name, &prepared, None); let (outcome, cause) = classify(&result); drop(prepared); - logs::ended(&outcome); - logs::complete(&outcome, cause.as_deref()); + logs::ended(outcome); + logs::complete(outcome, cause.as_deref()); notify(notification, outcome, cause.as_deref()); result } @@ -479,9 +485,11 @@ fn run_issue_branch( file, Some(issue.clone()), started, - &launch_git, - &commit, - Some(checkout.path()), + WorktreeSource { + git: &launch_git, + commit: &commit, + issue_checkout: Some(checkout.path()), + }, ) { Ok(prepared) => prepared, Err(error) => return Err(failed_before_steps(notification, error)), @@ -529,8 +537,8 @@ fn run_issue_branch( let (outcome, cause) = classify(&result); drop(prepared); drop(held); - logs::ended(&outcome); - logs::complete(&outcome, cause.as_deref()); + logs::ended(outcome); + logs::complete(outcome, cause.as_deref()); notify(notification, outcome, cause.as_deref()); result } @@ -556,10 +564,6 @@ fn execute_steps( .with_context(|| format!("could not create {}", results.display()))?; let mut earlier = Vec::new(); let mut last = String::new(); - let continuing = issue_run.as_ref().and_then(|run| { - run.on_origin - .then(|| (run.branch.to_string(), run.base.to_string())) - }); for range in group_ranges(&prepared.steps) { let group = &prepared.steps[range.clone()]; if group.len() == 1 && matches!(group[0].kind, StepKind::Delivery) { @@ -578,7 +582,7 @@ fn execute_steps( )?; continue; } - let checkout = issue_run.as_ref().and_then(|run| run.checkout.as_ref()); + let held = issue_run.as_deref(); let outcomes = if group.len() == 1 { let started = Instant::now(); let outcome = run_step( @@ -588,8 +592,7 @@ fn execute_steps( &earlier, &results, range.start == 0, - checkout, - step_continuation(prepared, &group[0], continuing.as_ref()), + held, ); vec![(started.elapsed(), outcome)] } else { @@ -600,8 +603,7 @@ fn execute_steps( &earlier, &results, range.start, - checkout, - continuing.as_ref(), + held, ) }; let mut group_error = None; @@ -652,9 +654,9 @@ fn run_parallel( earlier: &[(String, PathBuf)], results: &Path, group_start: usize, - checkout: Option<&Worktree>, - continuing: Option<&(String, String)>, + issue_run: Option<&IssueRun<'_>>, ) -> Vec<(Duration, Result)> { + let (checkout, continuing) = held_issue_branch(issue_run); thread::scope(|scope| { let mut handles = Vec::with_capacity(group.len()); for (offset, step) in group.iter().enumerate() { @@ -666,7 +668,7 @@ fn run_parallel( let first = group_start + offset == 0; let issue = prepared.issue.clone(); let started_at = prepared.started; - let continuation = step_continuation(prepared, step, continuing); + let continuation = step_continuation(prepared, step, continuing.as_ref()); handles.push(scope.spawn(move || { let started = Instant::now(); let run = StepRun { @@ -729,15 +731,15 @@ fn run_step( earlier: &[(String, PathBuf)], results: &Path, first: bool, - checkout: Option<&Worktree>, - continuation: Option<(String, String)>, + issue_run: Option<&IssueRun<'_>>, ) -> Result { + let (checkout, continuing) = held_issue_branch(issue_run); let run = StepRun { process, choice: &prepared.choice, issue: prepared.issue.as_ref(), started: prepared.started, - continuation, + continuation: step_continuation(prepared, step, continuing.as_ref()), }; run_step_in( &run, @@ -762,6 +764,20 @@ fn step_dir<'a>( worktree_path(&prepared.worktrees, step.worktree.as_deref()) } +/// The Issue-branch checkout a run holds, and the branch names when that +/// branch already has commits on origin. +fn held_issue_branch<'a>( + issue_run: Option<&'a IssueRun<'_>>, +) -> (Option<&'a Worktree>, Option<(String, String)>) { + let Some(run) = issue_run else { + return (None, None); + }; + let continuing = run + .on_origin + .then(|| (run.branch.to_string(), run.base.to_string())); + (run.checkout.as_ref(), continuing) +} + /// Branch and Base branch for a Step whose checkout continues the Issue branch. fn step_continuation( prepared: &Prepared, @@ -847,7 +863,7 @@ fn preserve_step_failure( ) { let reason = Cause::of(error).first_line().to_string(); let preserved = checkout.preserve_failed_run(&context.base, &reason, &mut || Ok(())); - let preservation = recovery::preservation_of(preserved, |line| progress::step(line)); + let preservation = recovery::preservation_of(preserved, progress::step); let mut recovery = Recovery::new(recovery::Stage::Implementation, preservation); recovery.record_worktree(checkout.path().to_path_buf()); recovery.add_command_context(context, on_origin); @@ -877,8 +893,10 @@ fn run_step_in( &step.name, name, path, - earlier, - first, + prompt::EarlierSteps { + results: earlier, + first, + }, run.issue, run.continuation .as_ref() @@ -1135,15 +1153,22 @@ fn load(name: &str) -> Result<(PathBuf, ProcessFile)> { Ok((path, file)) } +/// The launch checkout a Process's worktrees are created from, and the +/// Issue-branch checkout a run already holds. +struct WorktreeSource<'a> { + git: &'a Git, + commit: &'a str, + /// Reuse this checkout for the Issue-branch worktree instead of creating it. + issue_checkout: Option<&'a Path>, +} + fn prepare_from( path: &Path, config: &UserConfig, file: ProcessFile, issue: Option, started: DateTime, - launch: &Git, - commit: &str, - issue_checkout: Option<&Path>, + source: WorktreeSource<'_>, ) -> Result { let issue_worktree = file .worktrees @@ -1157,11 +1182,16 @@ fn prepare_from( issue_branch: worktree.issue_branch_line.is_some(), }) .collect(); - let skip = issue_checkout.and(issue_worktree.as_deref()); - let worktrees = create_worktrees(launch, commit, &file, skip)?; + let skip = source.issue_checkout.and(issue_worktree.as_deref()); + let worktrees = create_worktrees(source.git, source.commit, &file, skip)?; let mut steps = Vec::new(); for step in &file.steps { - let checkout = resolve_dir(&worktrees, step, issue_worktree.as_deref(), issue_checkout); + let checkout = resolve_dir( + &worktrees, + step, + issue_worktree.as_deref(), + source.issue_checkout, + ); steps.push(resolve(path, step, checkout)?); } let mut choice = Choice::of(&Asked::default(), &config.harness); diff --git a/src/prompt.rs b/src/prompt.rs index 32c546c8..533adfb8 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -115,6 +115,13 @@ pub struct ProcessContinuation<'a> { pub base: &'a str, } +/// Finished Steps named in a later Step's prompt, and whether that Step is +/// the Process's first. A later Step with no finished result is not the first. +pub struct EarlierSteps<'a> { + pub results: &'a [(String, std::path::PathBuf)], + pub first: bool, +} + /// The sentence a Continuation uses so the session builds on existing commits. fn continuing_work(branch: &str, base: &str) -> String { format!( @@ -122,20 +129,17 @@ fn continuing_work(branch: &str, base: &str) -> String { ) } -/// One Step of a Custom process. `earlier` is each finished Step's name and -/// the path of its final message, outside the worktree. `first` is whether -/// this is the Process's first Step. A later Step with no finished result -/// does not claim to be the first. `issue`, when the Process has one, is -/// named for the session to work from its file. `continuation`, when the -/// Step's checkout is an existing Issue branch, tells the session to build -/// on that branch's commits. +/// One Step of a Custom process. `earlier` names each finished Step and +/// whether this is the Process's first Step. `issue`, when the Process has +/// one, is named for the session to work from its file. `continuation`, when +/// the Step's checkout is an existing Issue branch, tells the session to +/// build on that branch's commits. pub fn process_step( process: &str, step: &str, skill_name: &str, skill_path: &std::path::Path, - earlier: &[(String, std::path::PathBuf)], - first: bool, + earlier: EarlierSteps<'_>, issue: Option<&IssueUrl>, continuation: Option>, ) -> String { @@ -143,8 +147,8 @@ pub fn process_step( .as_ref() .map(|note| format!("{}\n", continuing_work(note.branch, note.base))) .unwrap_or_default(); - let mut earlier_text = if earlier.is_empty() { - if first && continuation.is_none() { + let mut earlier_text = if earlier.results.is_empty() { + if earlier.first && continuation.is_none() { "This is the first Step. No earlier Step has a result.\n".to_string() } else { "No earlier Step has a result.\n".to_string() @@ -152,7 +156,7 @@ pub fn process_step( } else { String::new() }; - for (name, path) in earlier { + for (name, path) in earlier.results { let _ = writeln!( earlier_text, "Earlier Step result {name}: {}", @@ -716,8 +720,10 @@ mod tests { "gather", "to-spec", std::path::Path::new("skills/to-spec/SKILL.md"), - &[], - true, + EarlierSteps { + results: &[], + first: true, + }, Some(&issue), None, ), @@ -740,8 +746,10 @@ mod tests { "gather", "to-spec", std::path::Path::new("skills/to-spec/SKILL.md"), - &[], - true, + EarlierSteps { + results: &[], + first: true, + }, Some(&issue), None, ); @@ -754,8 +762,10 @@ mod tests { "gather", "to-spec", std::path::Path::new("skills/to-spec/SKILL.md"), - &[], - true, + EarlierSteps { + results: &[], + first: true, + }, None, None, ); diff --git a/src/prompts_page.rs b/src/prompts_page.rs index cb1c993a..a6828b65 100644 --- a/src/prompts_page.rs +++ b/src/prompts_page.rs @@ -331,8 +331,10 @@ fn prompts() -> Vec { STEP, SKILL_NAME, std::path::Path::new(SKILL_PATH), - &[], - true, + prompt::EarlierSteps { + results: &[], + first: true, + }, None, None, ), @@ -347,11 +349,13 @@ fn prompts() -> Vec { STEP, SKILL_NAME, std::path::Path::new(SKILL_PATH), - &[( - EARLIER_STEP.to_string(), - std::path::PathBuf::from(EARLIER_RESULT), - )], - false, + prompt::EarlierSteps { + results: &[( + EARLIER_STEP.to_string(), + std::path::PathBuf::from(EARLIER_RESULT), + )], + first: false, + }, None, None, ), @@ -366,8 +370,10 @@ fn prompts() -> Vec { STEP, SKILL_NAME, std::path::Path::new(SKILL_PATH), - &[], - true, + prompt::EarlierSteps { + results: &[], + first: true, + }, Some(&issue), None, ),