Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ From a clone of the issue's repository, `thirdshift <Issue URL>`:
3. Creates a git worktree next to your clone, named `<repo>-<Issue branch>`, so your own checkout is never touched.
4. Runs a headless Claude Code session in that worktree with the **Factory skills** in [`skills/`](skills/) loaded, started with one of the **Session prompts** in [`prompts/`](prompts/). The agent implements the issue, reviews its work against the Base branch, addresses the **Standards findings** and **Spec findings** it agrees with, and opens a ready-for-review pull request that lists every **Unaddressed finding** with a reason and closes the issue.
5. Takes over deterministically: pushes the Issue branch, skipping your repo's git hooks since the session runs the tests and CI gates the pull request, checks through `gh` that the pull request exists, is open and targets the Base branch, and marks it ready for review (`gh pr ready`) if the agent left it as a draft.
6. Keeps the pull request mergeable and green: merges the Base branch in and watches CI, starting a **Repair** session for a merge conflict or failing checks, at most 5 per Run. A failed check that also failed, under the same name, on the Base branch commit the head last merged in is an **Inherited failure**, not the branch's to fix: the CI-fix Repair is given only the branch's own failures, with the Inherited failures listed as not to fix. If every failed check is an Inherited failure, no Repair starts: the Run merges the Base branch again if it has moved since, and otherwise ends as a [Failed run](#failed-runs) that says to fix the Base branch first. A check that is pending, passed or missing on that Base branch commit is the branch's own, and thirdshift never triggers or waits for the Base branch's CI. If a CI-fix Repair leaves the head unchanged, with no commit of its own and no Base branch move to merge, the Run ends as a Failed run, since CI would stay red on the same head. If the Base branch moves while CI runs, it merges it again and goes round, at most 5 times per Run. In a Merge run, each round also takes in **Foreign commits** first: see [Foreign commits in a Merge run](#foreign-commits-in-a-merge-run).
6. Keeps the pull request mergeable and green: merges the Base branch in and watches CI, starting a **Repair** session for a merge conflict or failing checks, at most 5 per Run. A failed check that also failed, under the same name, on the Base branch commit the head last merged in is an **Inherited failure**, not the branch's to fix: the CI-fix Repair is given only the branch's own failures, with the Inherited failures listed as not to fix. If every failed check is an Inherited failure, no Repair starts: the Run merges the Base branch again if it has moved since, and otherwise ends as a [Failed run](#failed-runs) that says to fix the Base branch first, unless it was given `base-fix`, when it starts a [Base fix](#base-fix) first. A check that is pending, passed or missing on that Base branch commit is the branch's own, and thirdshift never triggers or waits for the Base branch's CI. If a CI-fix Repair leaves the head unchanged, with no commit of its own and no Base branch move to merge, the Run ends as a Failed run, since CI would stay red on the same head. If the Base branch moves while CI runs, it merges it again and goes round, at most 5 times per Run. In a Merge run, each round also takes in **Foreign commits** first: see [Foreign commits in a Merge run](#foreign-commits-in-a-merge-run).
7. In a **Merge run** (`thirdshift merge <Issue URL>`), does the **Self-merge**: once the pull request is open, ready for review, mergeable and green, thirdshift merges it into the Base branch with a merge commit, on exactly the head commit whose CI it watched (`gh pr merge --merge --match-head-commit <sha>`). It never uses GitHub's auto-merge ([ADR-0004](docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md)). A merge that fails goes back round step 6, within the same budgets, and is tried again on the new green head. If that round finds nothing to fix, the refusal is a **policy refusal**, such as merge commits being disallowed or a review being required. After the merge, thirdshift deletes the Issue branch on `origin`, and closes the issue if it is still open, with the comment `Closed by #<pr>, merged into <base> by a thirdshift Merge run.` GitHub closes it on its own only for a merge into the repository's default branch, and then thirdshift leaves it alone.
8. Cleans up: removes the worktree, the local Issue branch and the temporary plugin directory, whether the Run succeeded or not. The one exception is a **Failed run** whose work could not be pushed: see below.

Expand Down Expand Up @@ -280,6 +280,26 @@ Review Repairs are logged as `repair-<i>`, numbered with the other Repairs, coun

A Run without `merge` does none of this: it never fetches someone else's commits into its branch.

## Base fix

A Base branch whose CI is broken fails every Run on it with an Inherited failure. To have a Run fix the Base branch itself and carry on, give it `base-fix` (or `--base-fix`), before or after the URL:

```sh
thirdshift base-fix https://github.com/acme/widgets/issues/7
```

When the Run's only red checks are Inherited failures and the Base branch has not moved since, it then starts a **Base fix**, at most one per Run ([ADR-0008](docs/adr/0008-inherited-failures-fail-the-run.md)):

1. thirdshift opens an issue from a fixed template, with no agent session: titled `CI red on <base>: <check>[, <check>…]`, its body naming the checks with their URLs on the Base branch, the Base branch and its short sha, and the Run's pull request, labelled `base-fix` and `ready-for-agent`. A label the repository lacks is added to it first.
2. It starts a child `thirdshift` on that issue from the same Launch directory, as a Merge run into the Run's Base branch, whatever the Run's own goal. Its progress lines are relayed with a `#<n>: ` prefix, after `starting Base fix #<n> into <base>: <issue URL>` and `waiting on Base fix #<n>`. The Base fix sends no Run notification, treats every red check as its own to fix rather than as an Inherited failure, and never starts a Base fix of its own.
3. Once the Base fix has merged, and its Self-merge has closed its issue, the Run merges the Base branch in again and watches CI, and goes on as usual: ready for review, or merged in a Merge run.

If the Base fix fails, the Run is a [Failed run](#failed-runs) with the cause `Base fix <issue URL> failed: <its cause>`. If the checks are still Inherited failures once the Base fix has merged, there is no second one, and the cause is `CI red on <check>, which also fails on <base> at <short sha>, even after Base fix <issue URL> merged; fix <base> first`. The Run's own [Run notification](#run-notifications) has a `Base fix:` line with the issue's URL and whether it merged.

On a Spec, `base-fix` goes to each Ticket's Run, whose Base fix goes into the Spec branch, its Base branch, and to the Spec PR, whose Base fix goes into the Base branch. Tickets running at once that meet the same Inherited failure each start a Base fix of their own.

Without `base-fix`, an Inherited failure fails the Run as described in [What a Run does](#what-a-run-does).

## Continuation

Running thirdshift again on an issue picks up where the last Run stopped ([ADR-0002](docs/adr/0002-existing-issue-branch-means-continue.md)). It looks at the highest-numbered Issue branch:
Expand Down
39 changes: 29 additions & 10 deletions src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::num::NonZeroUsize;

use anyhow::{Context, Result, bail};

use crate::child_run::Kind;
use crate::issue::IssueUrl;
use crate::notification::NotificationAsk;
use crate::run::Goal;
Expand All @@ -21,11 +22,13 @@ pub enum Command {
}

/// The hidden argument a Spec run starts each Ticket's Run with, followed by
/// the Spec branch: it makes the Run a Merge run into the Spec branch that
/// sends no Run notification and leaves the Launch directory alone. Not in
/// help.
/// the Spec branch: it makes the Run a [`Kind::Ticket`]. Not in help.
pub const SPEC_BRANCH: &str = "--spec-branch";

/// The hidden argument a Run starts its Base fix with, followed by the Run's
/// Base branch: it makes the Run a [`Kind::BaseFix`]. Not in help.
pub const BASE_FIX_INTO: &str = "--base-fix-into";

/// A Run's arguments.
pub struct RunArgs {
pub issue: IssueUrl,
Expand All @@ -38,8 +41,11 @@ pub struct RunArgs {
/// How many Tickets a Spec run runs at once, if `parallel <n>` was
/// given; without it, the User config decides.
pub parallel: Option<NonZeroUsize>,
/// The Spec branch, given with [`SPEC_BRANCH`] to a Ticket's Run.
pub spec_branch: Option<String>,
/// Whether `base-fix` was given: the Run may start a Base fix.
pub base_fix: bool,
/// What the Run is, if another thirdshift started it, given with
/// [`SPEC_BRANCH`] or [`BASE_FIX_INTO`].
pub child: Option<Kind>,
}

/// Parse the arguments after the program name. `help`, `version`, `update`,
Expand Down Expand Up @@ -73,7 +79,8 @@ pub fn parse(args: &[String]) -> Result<Command> {
let mut goal = None;
let mut email = None;
let mut parallel = None;
let mut spec_branch = None;
let mut base_fix = false;
let mut child = None;
let mut args = args.iter().peekable();
while let Some(arg) = args.next() {
match arg.as_str() {
Expand All @@ -99,11 +106,22 @@ pub fn parse(args: &[String]) -> Result<Command> {
};
parallel = Some(n);
}
SPEC_BRANCH => {
if spec_branch.is_some() {
"base-fix" | "--base-fix" => {
if base_fix {
bail!("repeated argument: {arg}");
}
base_fix = true;
}
SPEC_BRANCH | BASE_FIX_INTO => {
if child.is_some() {
bail!("repeated argument: {arg}");
}
spec_branch = Some(args.next().context("missing Spec branch")?.clone());
let base = args.next().context("missing Base branch")?.clone();
child = Some(if arg == SPEC_BRANCH {
Kind::Ticket { spec_branch: base }
} else {
Kind::BaseFix { base }
});
}
_ => {
if issue.is_some() {
Expand All @@ -121,7 +139,8 @@ pub fn parse(args: &[String]) -> Result<Command> {
goal,
email,
parallel,
spec_branch,
base_fix,
child,
}))
}

Expand Down
186 changes: 186 additions & 0 deletions src/base_fix.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
//! The Base fix (ADR-0008): a Merge run into a Run's Base branch, on an issue
//! thirdshift writes, that a Run asked to starts when its only red checks are
//! Inherited failures, and waits for.

use anyhow::{Result, bail};

use crate::child_run::{self, Ended, Kind};
use crate::ci;
use crate::github::{self, Check, CheckState};
use crate::issue::IssueUrl;
use crate::progress;

/// The labels of a Base fix issue, each with the description it is added to
/// the repository with if the repository lacks it.
const LABELS: [(&str, &str); 2] = [
("base-fix", "A Base fix: CI is red on a Base branch"),
("ready-for-agent", "Ready for an agent to take on"),
];

/// What a Run does when its only red checks are Inherited failures.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum OnInheritedFailures {
/// Fail, saying to fix the Base branch first.
FailTheRun,
/// Start a Base fix, once, and go on when it has merged.
StartBaseFix,
/// The Run is itself a Base fix: every red check is its own to fix, so it
/// sees no Inherited failures and starts no Base fix.
IsBaseFix,
}

/// A Run's one Base fix: whether it may start one, and the one it started.
pub struct BaseFix {
on_inherited_failures: OnInheritedFailures,
started: Option<Started>,
}

/// A Base fix a Run started.
struct Started {
issue: IssueUrl,
merged: bool,
}

impl BaseFix {
/// The Base fix of a Run that is the child Run `child`, if it is one,
/// and that, with `asked`, was given `base-fix`. A Base fix starts none
/// of its own, whatever it was given.
pub fn new(child: Option<&Kind>, asked: bool) -> Self {
let on_inherited_failures = match (child, asked) {
(Some(Kind::BaseFix { .. }), _) => OnInheritedFailures::IsBaseFix,
(_, true) => OnInheritedFailures::StartBaseFix,
(_, false) => OnInheritedFailures::FailTheRun,
};
BaseFix {
on_inherited_failures,
started: None,
}
}

/// Whether the Run compares its red checks with the Base branch's, so
/// that some may be Inherited failures.
pub fn sees_inherited_failures(&self) -> bool {
self.on_inherited_failures != OnInheritedFailures::IsBaseFix
}

/// Whether the Run may start a Base fix, as may then each Ticket's Run
/// of a Spec run.
pub fn may_start(&self) -> bool {
self.on_inherited_failures == OnInheritedFailures::StartBaseFix
}

/// Fix the checks `inherited`, the only red ones on the PR `pr_url` of
/// the Run on `issue` and all Inherited failures from `base` at
/// `base_commit`, with a Base fix: write its issue, start it as a child
/// `thirdshift` and wait for it to merge, after which the Run is to merge
/// `base` in and watch CI again. Fails, with the Run's cause, if the Run
/// was not asked to start one, if the Base fix fails, in which case the
/// cause names its issue, or if the Run has had its one Base fix, in
/// which case the cause names that one's issue.
pub fn fix(
&mut self,
issue: &IssueUrl,
pr_url: &str,
base: &str,
base_commit: &str,
inherited: &[Check],
) -> Result<()> {
let checks = ci::check_names(inherited);
let base_at = ci::short(base_commit);
if let Some(started) = &self.started {
bail!(
"CI red on {checks}, which also fails on {base} at {base_at}, \
even after Base fix {} merged; fix {base} first",
started.issue.url
);
}
if !self.may_start() {
bail!("CI red on {checks}, which also fails on {base} at {base_at}; fix {base} first");
}
// The issue links the Base branch's own failed checks, not the PR's.
let on_base: Vec<Check> = github::checks_on(issue, base_commit)?
.into_iter()
.filter(|check| {
check.state == CheckState::Failed
&& inherited.iter().any(|failed| failed.name == check.name)
})
.collect();
let fix = github::create_issue(
issue,
&format!("CI red on {base}: {checks}"),
&issue_body(issue, pr_url, base, base_at, &on_base),
&LABELS,
)?;
let number = fix.number;
progress::step(format_args!(
"starting Base fix #{number} into {base}: {}",
fix.url
));
let started = self.started.insert(Started {
issue: fix,
merged: false,
});
let kind = Kind::BaseFix {
base: base.to_string(),
};
let child = child_run::start(&started.issue, &kind, false)?;
progress::step(format_args!("waiting on Base fix #{number}"));
match child_run::wait(number, child)? {
Ended::Reached(_) => {
started.merged = true;
progress::step(format_args!(
"Base fix #{number} merged; merging {base} in again"
));
Ok(())
}
Ended::Interrupted => bail!("interrupted"),
Ended::Failed { cause, log } => {
if let Some(log) = log {
progress::step(format_args!("Base fix #{number} session log: {log}"));
}
bail!("Base fix {} failed: {cause}", started.issue.url)
}
}
}

/// What became of the Base fix the Run started, if it started one, for
/// its Run notification: the issue's URL, then `merged` or `not merged`.
pub fn report(&self) -> Option<String> {
let started = self.started.as_ref()?;
let outcome = if started.merged {
"merged"
} else {
"not merged"
};
Some(format!("{} {outcome}", started.issue.url))
}
}

/// The body of the Base fix issue, from a fixed template: the checks
/// `failed` on the Base branch `base` at its commit `base_at`, shortened, with
/// their URLs, and the Run that found them, on `issue` with the PR `pr_url`.
fn issue_body(
issue: &IssueUrl,
pr_url: &str,
base: &str,
base_at: &str,
failed: &[Check],
) -> String {
let mut checks = String::new();
for check in failed {
checks += &match &check.url {
Some(url) => format!("- {}: {url}\n", check.name),
None => format!("- {}\n", check.name),
};
}
format!(
"CI is red on `{base}` at {base_at}: these checks fail there, so every pull request into `{base}` inherits them.\n\
\n\
{checks}\
\n\
Found by the thirdshift Run on #{}, whose pull request is {pr_url}.\n\
\n\
Fix the checks on `{base}`. Do not skip, disable, or weaken tests or checks to make them pass.\n",
issue.number
)
}
Loading
Loading