chadtmiller.com/knotless
git that lives in your Atmosphere account
1
10
A branch waits for a review agent's verdict before its pulls land
.changeset/review-gates.mdnew+7−0
| @@ -0,0 +1,7 @@ | ||
--- | ||
default: minor | ||
--- | ||
| ||
# A branch waits for a review agent's verdict with `knotless protect --gate` | ||
| ||
`knotless protect <url> <branch> --gate <handle>` names an account, such as a review agent, whose review of a pull's head the branch waits for, though it may not write to the repository. The merge queue holds a pull until the gate has reviewed its head, and a request for changes from it blocks merging and queueing until it reviews again. An approval of the head from the owner or a collaborator, given after the gate's review, stands in for it. `knotless rules` lists each gate, `merge --check` says `a review of its head from @<gate>` while one is awaited, and MCP's `protect_branch` takes `gates`. knotless-run's `--review-wait <secs>` (30 minutes) is how long the queue waits before ejecting a pull no gate has reviewed. | ||
crates/knotless/src/approvals.rs+141−6
| @@ -8,6 +8,12 @@ | ||
//! them blocks it. With `dismissStale`, only an approval of the pull's head | ||
//! as it is now counts; without, any standing approval does. | ||
//! | ||
//! With `gates`, the pull waits for a review of its head from each account | ||
//! named, as a review agent's ([`gated`]), though it may not write: one | ||
//! that asks for changes blocks it until it reviews again, and its | ||
//! approval counts toward nothing else. A collaborator's approval of the | ||
//! head, given after the gate's newest review, stands in for it. | ||
//! | ||
//! `knotless merge`, `merge --check`, the pages' merge box, the merge | ||
//! queue as it takes a pull in and as its runner puts one in a batch all | ||
//! read it here, so they agree. The owner's `--force` merges past it. | ||
| @@ -39,6 +45,9 @@ pub struct Standing { | ||
/// Those who may write whose standing review asks for changes: by | ||
/// handle, once [`of`] has read them, else by DID. | ||
pub changes: Vec<String>, | ||
/// The gates ([`gated`]) that have not reviewed the pull's head: by | ||
/// handle, once [`of`] has read them, else by DID. | ||
pub awaiting: Vec<String>, | ||
/// What the rule asks that this version does not know. | ||
pub unknown: Vec<String>, | ||
/// What reading the CODEOWNERS file skipped, and why. | ||
| @@ -102,6 +111,9 @@ impl Standing { | ||
for who in &self.changes { | ||
missing.push(format!("changes asked by {who}")); | ||
} | ||
for who in &self.awaiting { | ||
missing.push(format!("a review of its head from {who}")); | ||
} | ||
if !self.unknown.is_empty() { | ||
missing.push(format!( | ||
"what this version does not know: {}; update knotless", | ||
| @@ -111,6 +123,16 @@ impl Standing { | ||
missing | ||
} | ||
| ||
/// Whether all it lacks is a gate's review of its head: the merge | ||
/// queue holds such a pull rather than eject it. | ||
pub fn awaits(&self) -> bool { | ||
let rest = Standing { | ||
awaiting: Vec::new(), | ||
..self.clone() | ||
}; | ||
!self.awaiting.is_empty() && rest.passed() | ||
} | ||
| ||
/// The approvals that do not count, and why, each in a line. | ||
pub fn uncounted(&self) -> Vec<String> { | ||
let said = self.uncounted.iter(); | ||
| @@ -119,6 +141,36 @@ impl Standing { | ||
} | ||
} | ||
| ||
/// How the gates of `rule` stand on a pull at `head` with `reviews`, each | ||
/// reviewer's standing review in the order they last reviewed, `writer` | ||
/// saying who may write to the repository: those asking for changes, then | ||
/// those that have not reviewed `head`. A writer's approval of `head` | ||
/// later than a gate's newest review, or with none, stands in for that | ||
/// gate's. | ||
pub fn gated( | ||
rule: &Reviews, | ||
reviews: &[Review], | ||
head: &str, | ||
writer: impl Fn(&str) -> bool, | ||
) -> (Vec<String>, Vec<String>) { | ||
let (mut changes, mut awaiting) = (Vec::new(), Vec::new()); | ||
for gate in &rule.gates { | ||
let at = reviews.iter().position(|r| r.by == gate.did); | ||
let after = &reviews[at.map_or(0, |at| at + 1)..]; | ||
let stood_in = | ||
|r: &Review| r.verdict == reviews::APPROVE && r.at_head(head) && writer(&r.by); | ||
if after.iter().any(stood_in) { | ||
continue; | ||
} | ||
match at.map(|at| &reviews[at]) { | ||
Some(r) if r.verdict == reviews::CHANGES => changes.push(gate.did.clone()), | ||
Some(r) if r.at_head(head) => {} | ||
_ => awaiting.push(gate.did.clone()), | ||
} | ||
} | ||
(changes, awaiting) | ||
} | ||
| ||
/// How `reviews`, each reviewer's standing review of a pull by `author` | ||
/// whose head is `head`, stand against `rule`, `writer` saying who may write to the | ||
/// repository. `owners` is the base's CODEOWNERS, if it has one, and | ||
| @@ -153,16 +205,26 @@ pub fn stand( | ||
} | ||
} | ||
} | ||
let changes = (reviews.iter()) | ||
let mut changes: Vec<String> = (reviews.iter()) | ||
.filter(|r| writer(&r.by) && r.verdict == reviews::CHANGES) | ||
.map(|r| r.by.clone()); | ||
.map(|r| r.by.clone()) | ||
.collect(); | ||
let (gate_changes, awaiting) = gated(rule, reviews, head, &writer); | ||
for gate in gate_changes { | ||
if !changes.contains(&gate) { | ||
changes.push(gate); | ||
} | ||
} | ||
let gate_unknown = | ||
(rule.gates.iter()).flat_map(|g| g.unknown.keys().map(|k| format!("gates.{k}"))); | ||
Standing { | ||
approvals: approving.len(), | ||
required: rule.approvals as usize, | ||
owners: unowned, | ||
authored, | ||
changes: changes.collect(), | ||
unknown: rule.unknown.keys().cloned().collect(), | ||
changes, | ||
awaiting, | ||
unknown: rule.unknown.keys().cloned().chain(gate_unknown).collect(), | ||
warnings: file.map(|f| f.warnings.clone()).unwrap_or_default(), | ||
uncounted: not_counted.collect(), | ||
} | ||
| @@ -202,10 +264,13 @@ pub async fn of( | ||
&changed, | ||
); | ||
let named = standing.uncounted.iter().map(|(who, _)| who); | ||
let all = standing.changes.iter().chain(named).map(String::as_str); | ||
let all = (standing.changes.iter().chain(&standing.awaiting)) | ||
.chain(named) | ||
.map(String::as_str); | ||
let handles = identity::handles(ctx, all).await; | ||
let uncounted = standing.uncounted.iter_mut().map(|(who, _)| who); | ||
for who in standing.changes.iter_mut().chain(uncounted) { | ||
let shown = standing.changes.iter_mut().chain(&mut standing.awaiting); | ||
for who in shown.chain(uncounted) { | ||
if let Some(handle) = handles.get(who.as_str()) { | ||
*who = format!("@{handle}"); | ||
} | ||
| @@ -335,6 +400,76 @@ mod tests { | ||
assert!(why.contains(&said), "{why}"); | ||
} | ||
| ||
#[test] | ||
fn a_gate_is_waited_for_and_its_changes_block() { | ||
use crate::records::Gate; | ||
let writer = |did: &str| did == "did:alice"; | ||
let gate = |did: &str| Gate { | ||
did: did.into(), | ||
unknown: Default::default(), | ||
}; | ||
let mut rule = Reviews { | ||
gates: vec![gate("did:bot")], | ||
..Reviews::default() | ||
}; | ||
let s = |reviews: &[Review], rule: &Reviews| { | ||
stand(rule, "did:dan", reviews, "head", writer, None, &[]) | ||
}; | ||
// Not yet reviewed, or reviewed at an earlier head: it waits. | ||
let none = s(&[], &rule); | ||
assert_eq!(none.missing(), ["a review of its head from did:bot"]); | ||
assert!(none.awaits()); | ||
let old = s(&[review("did:bot", reviews::APPROVE, "old")], &rule); | ||
assert!(old.awaits()); | ||
// Its approval of the head lets it go, and counts toward nothing. | ||
let ok = s(&[review("did:bot", reviews::APPROVE, "head")], &rule); | ||
assert!(ok.passed() && ok.approvals == 0); | ||
// Its changes block, at any head, and are no wait. | ||
let asked = [review("did:bot", reviews::CHANGES, "old")]; | ||
let blocked = s(&asked, &rule); | ||
assert_eq!(blocked.missing(), ["changes asked by did:bot"]); | ||
assert!(!blocked.awaits()); | ||
// A writer's approval of the head after it stands in; one before | ||
// it, or of an earlier head, does not. | ||
let after = [ | ||
asked[0].clone(), | ||
review("did:alice", reviews::APPROVE, "head"), | ||
]; | ||
assert!(s(&after, &rule).passed()); | ||
let before = [after[1].clone(), after[0].clone()]; | ||
assert!(!s(&before, &rule).passed()); | ||
let stale = [ | ||
asked[0].clone(), | ||
review("did:alice", reviews::APPROVE, "old"), | ||
]; | ||
assert!(!s(&stale, &rule).passed()); | ||
// A stranger's approval stands in for nothing. | ||
let stranger = [ | ||
asked[0].clone(), | ||
review("did:eve", reviews::APPROVE, "head"), | ||
]; | ||
assert!(!s(&stranger, &rule).passed()); | ||
// Waiting beside something else missing is no mere wait. | ||
rule.approvals = 1; | ||
assert!(!s(&[], &rule).awaits()); | ||
// A gate a writer also is counts its changes once. | ||
let rule = Reviews { | ||
gates: vec![gate("did:alice")], | ||
..Reviews::default() | ||
}; | ||
let own = s(&[review("did:alice", reviews::CHANGES, "head")], &rule); | ||
assert_eq!(own.changes, ["did:alice"]); | ||
// What a newer client said of a gate blocks until known. | ||
let mut newer = gate("did:bot"); | ||
newer.unknown.insert("rounds".into(), 2.into()); | ||
let rule = Reviews { | ||
gates: vec![newer], | ||
..Reviews::default() | ||
}; | ||
let s = s(&[review("did:bot", reviews::APPROVE, "head")], &rule); | ||
assert_eq!(s.unknown, ["gates.rounds"]); | ||
} | ||
| ||
#[test] | ||
fn owners_changes_and_what_is_not_known() { | ||
let writer = |did: &str| did != "did:stranger"; | ||
crates/knotless/src/cli.rs+16−3
| @@ -131,7 +131,7 @@ const USAGE: &str = "usage: knotless <command> | ||
a member of in public | ||
org delete --yes take every member out: your account is no | ||
longer an organization | ||
protect <url> <branch> [--merges] [--queue] [--reviews <n>] [--code-owners] [--dismiss-stale] | ||
protect <url> <branch> [--merges] [--queue] [--reviews <n>] [--code-owners] [--dismiss-stale] [--gate <handle>...] | ||
hold collaborators to moving it forward, | ||
never deleting it, and, without --merges, | ||
no merge commits (pulls merge squashed); | ||
| @@ -142,7 +142,9 @@ const USAGE: &str = "usage: knotless <command> | ||
--code-owners once an owner of each path | ||
it changes does (CODEOWNERS), with | ||
--dismiss-stale only approvals of its | ||
head counting | ||
head counting, with --gate waiting for | ||
that account's review of its head, its | ||
changes blocking it | ||
unprotect <url> <branch> | ||
rules <url> the protected branches | ||
runners <url> [add|remove <handle>] the accounts whose checks of the | ||
| @@ -898,7 +900,7 @@ fn said_mergeable(out: &mut dyn Write, m: &crate::landing::Mergeable) -> Result< | ||
} | ||
| ||
/// What `rules` says a rule asks of reviews, each after a tab: `reviews | ||
/// <n>`, `code owners`, `dismiss stale`. | ||
/// <n>`, `code owners`, `dismiss stale`, and `gate <did>` for each gate. | ||
fn said_reviews(r: &records::Reviews) -> String { | ||
let mut said = format!("\treviews {}", r.approvals); | ||
if r.code_owners { | ||
| @@ -907,6 +909,9 @@ fn said_reviews(r: &records::Reviews) -> String { | ||
if r.dismiss_stale { | ||
said.push_str("\tdismiss stale"); | ||
} | ||
for gate in &r.gates { | ||
said.push_str(&format!("\tgate {}", gate.did)); | ||
} | ||
said | ||
} | ||
| ||
| @@ -1282,6 +1287,14 @@ async fn command( | ||
} | ||
"--code-owners" if protect => reviews.code_owners = true, | ||
"--dismiss-stale" if protect => reviews.dismiss_stale = true, | ||
"--gate" if protect => { | ||
let who = args.next("<handle>")?; | ||
let did = crate::identity::resolve(ctx, who).await?.did; | ||
reviews.gates.push(records::Gate { | ||
did, | ||
unknown: Default::default(), | ||
}); | ||
} | ||
other => bail!("unexpected {other}"), | ||
} | ||
} | ||
crates/knotless/src/forge.rs+3−4
| @@ -2885,11 +2885,10 @@ pub async fn list(ctx: &Ctx, target: &RepoUrl) -> Result<Vec<Item>> { | ||
named(ctx, &t, &t.entries(ctx).await?).await | ||
} | ||
| ||
/// The pulls and issues of `target`, as its merge queue reads them | ||
/// The pulls and issues of `t`, as its merge queue reads them | ||
/// ([`Target::queue_entries`]). | ||
pub async fn list_queue(ctx: &Ctx, target: &RepoUrl) -> Result<Vec<Item>> { | ||
let t = Target::open(ctx, target).await?; | ||
named(ctx, &t, &t.queue_entries(ctx).await?).await | ||
pub async fn list_queue(ctx: &Ctx, t: &Target) -> Result<Vec<Item>> { | ||
named(ctx, t, &t.queue_entries(ctx).await?).await | ||
} | ||
| ||
/// The pulls and issues `entries` make of `t`, each with its author's | ||
crates/knotless/src/queue.rs+73−7
| @@ -27,7 +27,7 @@ use crate::git::Git; | ||
use crate::keys; | ||
use crate::push::Update; | ||
use crate::records::{ | ||
self, Batched, FailedCheck, QueueRecord, StateRecord, nsid, queue::*, status, | ||
self, Batched, FailedCheck, QueueRecord, RepoRecord, StateRecord, nsid, queue::*, status, | ||
}; | ||
use crate::remote; | ||
use crate::session::{self, Session}; | ||
| @@ -215,7 +215,10 @@ async fn admit(ctx: &Ctx, t: &Target, item: &Item) -> Result<String> { | ||
files.join(", ") | ||
); | ||
} | ||
// One lacking only a gate's review of its head is queued to wait for | ||
// it ([`approvals::Standing::awaits`]). | ||
let reviewed = approvals::of(ctx, t.facts(), item, &sides.git, base, head).await?; | ||
let reviewed = reviewed.filter(|s| !s.awaits()); | ||
if let Some(why) = reviewed.and_then(|s| approvals::refusal(&t.url, &item.uri, into, &s)) { | ||
bail!("{why}. Queue it again once it is"); | ||
} | ||
| @@ -426,6 +429,9 @@ pub mod stage { | ||
pub const AGAIN: &str = "again"; | ||
/// Its pre-flights passed: it goes into the next batch. | ||
pub const READY: &str = "ready"; | ||
/// Its pre-flights passed, and it waits for a review of its head from | ||
/// its branch's gates ([`crate::approvals::gated`]). | ||
pub const REVIEW: &str = "review"; | ||
/// In a batch being checked. | ||
pub const CHECKING: &str = "checking"; | ||
/// In a batch that passed, landing. | ||
| @@ -456,6 +462,7 @@ impl Waiting { | ||
p.base | ||
), | ||
stage::READY => format!("{place}, ready for the next batch"), | ||
stage::REVIEW => format!("{place}, waiting for its review"), | ||
_ => place, | ||
}; | ||
} | ||
| @@ -582,8 +589,9 @@ fn stage_of(newest: &[&Check], all: &[&Check]) -> &'static str { | ||
/// and the pre-flights of the pulls among them: the queued pulls in order, | ||
/// each with its batch if one is being checked or landing, or else the | ||
/// pre-flights it waits for, and then those turned away that are still | ||
/// open. | ||
pub fn waiting(items: &[Item], checks: &[Check]) -> Vec<Waiting> { | ||
/// open. `record` is the repository's, owned by `owner`: one ready but | ||
/// for its gates' review of its head waits for it. | ||
pub fn waiting(items: &[Item], checks: &[Check], record: &RepoRecord, owner: &str) -> Vec<Waiting> { | ||
let batches = batches(checks); | ||
let mut waiting: Vec<Waiting> = queued(items) | ||
.into_iter() | ||
| @@ -599,6 +607,12 @@ pub fn waiting(items: &[Item], checks: &[Check]) -> Vec<Waiting> { | ||
Some(_) => stage::CHECKING, | ||
None => stage_of(&newest, &all), | ||
}; | ||
let stage = match newest.first() { | ||
Some(c) if stage == stage::READY && awaits(i, &c.record.commit, record, owner) => { | ||
stage::REVIEW | ||
} | ||
_ => stage, | ||
}; | ||
let going = newest.iter().filter(|c| status::going(&c.record.status)); | ||
let refname = format!("refs/heads/{}", i.base.as_deref().unwrap_or_default()); | ||
let checking = | ||
| @@ -770,10 +784,24 @@ pub async fn pull_checks(ctx: &Ctx, url: &RepoUrl, items: &[Item]) -> Vec<Check> | ||
/// pre-flights it waits for, and then those turned away that are still | ||
/// open. | ||
pub async fn list(ctx: &Ctx, url: &RepoUrl) -> Result<Vec<Waiting>> { | ||
let items = forge::list_queue(ctx, url).await?; | ||
let t = Target::open(ctx, url).await?; | ||
let items = forge::list_queue(ctx, &t).await?; | ||
let mut found = batch_checks(ctx, url, &branches(&items)).await; | ||
found.extend(pull_checks(ctx, url, &items).await); | ||
Ok(waiting(&items, &found)) | ||
Ok(waiting(&items, &found, &t.record, t.remote.did())) | ||
} | ||
| ||
/// Whether `item`, a pull at `head` of the repository whose record is | ||
/// `record`, owned by `owner`, waits for a gate's review of its head. | ||
fn awaits(item: &Item, head: &str, record: &RepoRecord, owner: &str) -> bool { | ||
let branch = item.base.as_deref().unwrap_or_default(); | ||
let Some(rule) = record.rule(branch).and_then(|r| r.reviews.as_ref()) else { | ||
return false; | ||
}; | ||
let writer = |did: &str| did == owner || record.is_collaborator(did); | ||
!approvals::gated(rule, &item.reviews, head, writer) | ||
.1 | ||
.is_empty() | ||
} | ||
| ||
/// Where `pull` stands in the merge queue of the repository at `url`, if | ||
| @@ -1171,7 +1199,7 @@ mod tests { | ||
.collect(); | ||
assert_eq!(landed, ["t7"]); | ||
| ||
let waiting = waiting(&items, &checks); | ||
let waiting = waiting(&items, &checks, &RepoRecord::default(), "did:o"); | ||
let said: Vec<String> = waiting | ||
.iter() | ||
.map(|w| format!("{}: {}", w.item.title, w.said())) | ||
| @@ -1191,6 +1219,44 @@ mod tests { | ||
assert_eq!(branches(&items), ["main"]); | ||
} | ||
| ||
#[test] | ||
fn a_pull_its_gate_has_not_reviewed_waits_for_its_review() { | ||
let gates = vec![records::Gate { | ||
did: "did:g".into(), | ||
unknown: Default::default(), | ||
}]; | ||
let reviews = records::Reviews { | ||
gates, | ||
..Default::default() | ||
}; | ||
let record = RepoRecord { | ||
rules: vec![records::Rule::new("main", false, true).reviewed(reviews)], | ||
..Default::default() | ||
}; | ||
let review = |by: &str| crate::reviews::Review { | ||
by: by.into(), | ||
verdict: crate::reviews::APPROVE.into(), | ||
commit: Some("h1".into()), | ||
uri: format!("at://{by}/dev.knotless.comment/c"), | ||
}; | ||
// a, unreviewed, waits; the gate approved b's head; the owner's | ||
// approval of c's head stands in for the gate's. | ||
let a = item("a", "open", queued_at("1")); | ||
let mut b = item("b", "open", queued_at("1")); | ||
b.reviews = vec![review("did:g")]; | ||
let mut c = item("c", "open", queued_at("1")); | ||
c.reviews = vec![review("did:o")]; | ||
let checks = [ | ||
flight("a1", "a", "h1", "success", "7"), | ||
flight("b1", "b", "h1", "success", "7"), | ||
flight("c1", "c", "h1", "success", "7"), | ||
]; | ||
let w = waiting(&[a, b, c], &checks, &record, "did:o"); | ||
let stages: Vec<&str> = w.iter().map(|w| w.stage.as_str()).collect(); | ||
assert_eq!(stages, ["review", "ready", "ready"]); | ||
assert_eq!(w[0].said(), "Queued, 1st of 3, waiting for its review"); | ||
} | ||
| ||
/// A run of a pre-flight of the pull `p`, at its head `commit`. | ||
fn flight(n: &str, p: &str, commit: &str, status: &str, at: &str) -> Check { | ||
let mut c = run(n, commit, &[], "test", status, at); | ||
| @@ -1243,7 +1309,7 @@ mod tests { | ||
unfinished, | ||
flight("g1", "g", "h1", "cancelled", "7"), | ||
]; | ||
let w = waiting(&items, &checks); | ||
let w = waiting(&items, &checks, &RepoRecord::default(), "did:o"); | ||
let stood: Vec<(&str, &str, Option<&str>)> = w | ||
.iter() | ||
.map(|w| (w.item.title.as_str(), w.stage.as_str(), w.ahead.as_deref())) | ||
crates/knotless/src/records.rs+15−0
| @@ -314,12 +314,27 @@ pub struct Reviews { | ||
/// Only an approval of the pull's head as it is now counts. | ||
#[serde(default, skip_serializing_if = "std::ops::Not::not")] | ||
pub dismiss_stale: bool, | ||
/// Accounts whose review of the pull's head it waits for, as a review | ||
/// agent's: one asking for changes blocks it, though it may not write | ||
/// to the repository, and its approval counts toward nothing. | ||
#[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
pub gates: Vec<Gate>, | ||
/// What a newer client asked of reviews, kept on a rewrite. This | ||
/// version merges into the branch only once it knows them. | ||
#[serde(flatten)] | ||
pub unknown: BTreeMap<String, Value>, | ||
} | ||
| ||
/// An account a branch's pulls wait for a review from ([`Reviews::gates`]). | ||
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq)] | ||
pub struct Gate { | ||
pub did: String, | ||
/// What a newer client said of it, kept on a rewrite. This version | ||
/// merges into the branch only once it knows it. | ||
#[serde(flatten)] | ||
pub unknown: BTreeMap<String, Value>, | ||
} | ||
| ||
fn is_zero(n: &u32) -> bool { | ||
*n == 0 | ||
} | ||
crates/knotless-mcp/src/lib.rs+16−3
| @@ -873,6 +873,12 @@ pub struct Protect { | ||
/// Only approvals of the pull's head as it is now count. | ||
#[serde(default)] | ||
pub dismiss_stale: bool, | ||
/// Accounts, by handle or DID, such as review agents, whose review of | ||
/// a pull's head it waits for, though they may not write: one asking | ||
/// for changes blocks it, and their approvals count toward nothing | ||
/// else. | ||
#[serde(default)] | ||
pub gates: Vec<String>, | ||
/// Stop protecting it instead. | ||
#[serde(default)] | ||
pub remove: bool, | ||
| @@ -1767,7 +1773,7 @@ impl Server { | ||
} | ||
| ||
#[tool( | ||
description = "Protect a branch of a repository you own, or with remove, stop: collaborators can then only move it forward and never delete it, and unless merges is set, put no merge commits on it. With queue, pulls land on it only through the merge queue: merge queues them instead unless the owner forces it, and the gateway moves it for no collaborator but the runner that lands the queue. The gateway holds them to it. With reviews, a pull merges into it only once that many of the owner and collaborators, other than its author, approve it (comment with verdict approve); with code_owners, only once, for each path it changes that the CODEOWNERS file on the branch (.knotless/CODEOWNERS, CODEOWNERS or .github/CODEOWNERS, the first there) gives owners, one of them approves it; with dismiss_stale, only approvals of its head as it is now count. A standing request for changes from the owner or a collaborator blocks it too, and the owner's force merges past all of it. Protecting a branch again replaces its rule. Answers the protected branches, as list_rules does." | ||
description = "Protect a branch of a repository you own, or with remove, stop: collaborators can then only move it forward and never delete it, and unless merges is set, put no merge commits on it. With queue, pulls land on it only through the merge queue: merge queues them instead unless the owner forces it, and the gateway moves it for no collaborator but the runner that lands the queue. The gateway holds them to it. With reviews, a pull merges into it only once that many of the owner and collaborators, other than its author, approve it (comment with verdict approve); with code_owners, only once, for each path it changes that the CODEOWNERS file on the branch (.knotless/CODEOWNERS, CODEOWNERS or .github/CODEOWNERS, the first there) gives owners, one of them approves it; with dismiss_stale, only approvals of its head as it is now count; with gates, accounts such as review agents, it waits for each one's review of its head, the merge queue holding it meanwhile (and ejecting it after the runner's --review-wait, 30 minutes unless set), and one of theirs asking for changes blocks it until that account reviews again, though they may not write; their approvals count toward nothing else, and an approval of the head from the owner or a collaborator, given after a gate's newest review, stands in for that gate's. A standing request for changes from the owner or a collaborator blocks it too, and the owner's force merges past all of it. Protecting a branch again replaces its rule. Answers the protected branches, as list_rules does." | ||
)] | ||
async fn protect_branch( | ||
&self, | ||
| @@ -1775,10 +1781,17 @@ impl Server { | ||
) -> Result<CallToolResult, McpError> { | ||
answer( | ||
async { | ||
let mut gates = Vec::new(); | ||
for who in &p.gates { | ||
let did = knotless::identity::resolve(&self.ctx, who).await?.did; | ||
let unknown = Default::default(); | ||
gates.push(knotless::records::Gate { did, unknown }); | ||
} | ||
let reviews = knotless::records::Reviews { | ||
approvals: p.reviews, | ||
code_owners: p.code_owners, | ||
dismiss_stale: p.dismiss_stale, | ||
gates, | ||
..Default::default() | ||
}; | ||
let rule = (!p.remove).then(|| { | ||
| @@ -2268,7 +2281,7 @@ impl Server { | ||
} | ||
| ||
#[tool( | ||
description = "Merge an open pull request: a fast-forward when possible, else a merge commit; into a branch protected as linear, one squashed commit. Fails on conflicts. In a repository that names runners, only once every workflow that runs on the pull has passed at its head merged with the target branch as it is now: it refuses, naming what failed, what is still running, or the older head or base its checks are of, and checks with pull and log shows why. With wait, it waits up to that many seconds for checks running or not yet run, and the owner's merge asks the runners once to run a head and base none has; force, the owner's alone, merges whatever they say. Into a branch protected with queue (list_rules), it queues the pull instead, as queue does, and answers queued with why, unless the owner forces it. Its head branch, when in this repository, is deleted with it unless keep_branch or another open pull names it. The owner and collaborators may. Into the repository's default branch, it closes each open issue on it that the pull's title or body names right after fixes, closes or resolves (or fix, fixed, close, closed, resolve, resolved), by its page URL (https://<appview>/<owner>/<repo>/issue/<did>/<rkey>) or at:// URI, returning them as closed; the merge queue does the same as it lands. With queue, it puts the pull in its branch's merge queue instead and returns at once with its place (queued: position of of), and, queued again at the very head the queue last ejected, why it was (queued.ejectedBefore), as it will be again unless that was not the pull's doing: the quickest way to land, as a runner checks queued pulls together a batch at a time, and there is no need to wait. Queue a pull once it builds, though: one that does not fails its whole batch, and every pull in it waits while the queue halves the batch to find it. Merge the target branch into the pull's branch just before queueing: one that conflicts with it is refused. Ejected, read why with show_item and the failed checks' logs with checks (pull and log), fix it, and queue it again; queue the same head again only when the log shows the failure is not the pull's, such as a test it does not touch that fails other pulls too. One that conflicts only with a pull ahead of it waits for that pull to land or leave, and is ejected then if it conflicts with the branch: merge the branch into it. Ejected again for the same reason, stop and say so rather than queue it once more. merge_queue shows where it is, and show_item whether it merged, or where it stands (standing.said: Queued, 3rd of 5, running its checks or ready for the next batch; In a batch of 4 being checked; Landing; or why it was ejected and which checks failed, also under item.queue.reason and item.queue.checks). Into a branch protected with reviews, it refuses until the pull is reviewed as the rule asks (see protect_branch), naming what is missing. With check, it merges nothing and says whether it would now: state clean, conflicting (conflicts, the paths) or blocked by its checks (checks: passed, failed, running, pending, or none when the repository names no runners) or its reviews (reviews: passed, blocked, or none when its branch asks for none; reviewsMissing, what they lack, as reviews 1 of 2 or owners of web/: @alice or @bob; reviewsUncounted, the approvals that do not count and why; codeOwnersWarnings, the CODEOWNERS lines skipped and why); reason, why not, in the words a merge would refuse with, and that you may not merge it when you may not; method (fastForward, merge or squash); deletes, the branch it would delete; and may, what your part in the repository lets you do to it (merge, force, queue, base and head as retarget_pull changes them, rerun): not whether it would work now, which state and reason say. A blocked pull's merge is refused, the owner's too, unless the owner forces it. queueOnly, when its branch takes pulls only through the merge queue. Merged, it answers the commit its branch is at now (sha), how (method: fastForward, merge or squash), the branch it deleted (deleted) or why it kept the head branch (kept: asked to, in another repository such as a fork, the branch it merges into or the default branch, or named by another open pull), and the issues it closed." | ||
description = "Merge an open pull request: a fast-forward when possible, else a merge commit; into a branch protected as linear, one squashed commit. Fails on conflicts. In a repository that names runners, only once every workflow that runs on the pull has passed at its head merged with the target branch as it is now: it refuses, naming what failed, what is still running, or the older head or base its checks are of, and checks with pull and log shows why. With wait, it waits up to that many seconds for checks running or not yet run, and the owner's merge asks the runners once to run a head and base none has; force, the owner's alone, merges whatever they say. Into a branch protected with queue (list_rules), it queues the pull instead, as queue does, and answers queued with why, unless the owner forces it. Its head branch, when in this repository, is deleted with it unless keep_branch or another open pull names it. The owner and collaborators may. Into the repository's default branch, it closes each open issue on it that the pull's title or body names right after fixes, closes or resolves (or fix, fixed, close, closed, resolve, resolved), by its page URL (https://<appview>/<owner>/<repo>/issue/<did>/<rkey>) or at:// URI, returning them as closed; the merge queue does the same as it lands. With queue, it puts the pull in its branch's merge queue instead and returns at once with its place (queued: position of of), and, queued again at the very head the queue last ejected, why it was (queued.ejectedBefore), as it will be again unless that was not the pull's doing: the quickest way to land, as a runner checks queued pulls together a batch at a time, and there is no need to wait. Queue a pull once it builds, though: one that does not fails its whole batch, and every pull in it waits while the queue halves the batch to find it. Merge the target branch into the pull's branch just before queueing: one that conflicts with it is refused. Ejected, read why with show_item and the failed checks' logs with checks (pull and log), fix it, and queue it again; queue the same head again only when the log shows the failure is not the pull's, such as a test it does not touch that fails other pulls too. One that conflicts only with a pull ahead of it waits for that pull to land or leave, and is ejected then if it conflicts with the branch: merge the branch into it. Ejected again for the same reason, stop and say so rather than queue it once more. merge_queue shows where it is, and show_item whether it merged, or where it stands (standing.said: Queued, 3rd of 5, running its checks, ready for the next batch or waiting for its review, from a gate its branch names; In a batch of 4 being checked; Landing; or why it was ejected and which checks failed, also under item.queue.reason and item.queue.checks). Into a branch protected with reviews, it refuses until the pull is reviewed as the rule asks (see protect_branch), naming what is missing. With check, it merges nothing and says whether it would now: state clean, conflicting (conflicts, the paths) or blocked by its checks (checks: passed, failed, running, pending, or none when the repository names no runners) or its reviews (reviews: passed, blocked, or none when its branch asks for none; reviewsMissing, what they lack, as reviews 1 of 2, owners of web/: @alice or @bob, or a review of its head from @bot; reviewsUncounted, the approvals that do not count and why; codeOwnersWarnings, the CODEOWNERS lines skipped and why); reason, why not, in the words a merge would refuse with, and that you may not merge it when you may not; method (fastForward, merge or squash); deletes, the branch it would delete; and may, what your part in the repository lets you do to it (merge, force, queue, base and head as retarget_pull changes them, rerun): not whether it would work now, which state and reason say. A blocked pull's merge is refused, the owner's too, unless the owner forces it. queueOnly, when its branch takes pulls only through the merge queue. Merged, it answers the commit its branch is at now (sha), how (method: fastForward, merge or squash), the branch it deleted (deleted) or why it kept the head branch (kept: asked to, in another repository such as a fork, the branch it merges into or the default branch, or named by another open pull), and the issues it closed." | ||
)] | ||
async fn merge( | ||
&self, | ||
| @@ -3071,7 +3084,7 @@ impl Server { | ||
} | ||
| ||
#[tool( | ||
description = "The protected branches of a repository, whether each lets merge commits in (history), whether pulls land on it only through the merge queue (queue), and the reviews a pull needs to merge into it (reviews: approvals, codeOwners, dismissStale; see protect_branch). A rule asking for code owners has warnings when CODEOWNERS on its branch gives you, the owner, a path alone: an author's approval never counts, so your own pull changing it merges only by force." | ||
description = "The protected branches of a repository, whether each lets merge commits in (history), whether pulls land on it only through the merge queue (queue), and the reviews a pull needs to merge into it (reviews: approvals, codeOwners, dismissStale, gates by DID; see protect_branch). A rule asking for code owners has warnings when CODEOWNERS on its branch gives you, the owner, a path alone: an author's approval never counts, so your own pull changing it merges only by force." | ||
)] | ||
async fn list_rules( | ||
&self, | ||
crates/knotless-mcp/tests/mcp.rs+6−0
| @@ -1500,6 +1500,12 @@ fn agents_fork_review_and_merge() { | ||
wait_until(|| o.json("merge_queue", json!({"repo": repo})) == json!([])); | ||
let unheld = json!({"repo": repo, "branch": "main", "remove": true}); | ||
assert_eq!(o.json("protect_branch", unheld.clone()), json!([])); | ||
// A gate named by its handle is kept by its DID. | ||
let gate = json!({"repo": repo, "branch": "main", "gates": [contrib.account.handle]}); | ||
let rules = o.json("protect_branch", gate); | ||
let gates = &rules[0]["reviews"]["gates"]; | ||
assert_eq!(gates[0]["did"], contrib.account.did.as_str(), "{rules}"); | ||
assert_eq!(o.json("protect_branch", unheld.clone()), json!([])); | ||
| ||
// Into a branch that asks for a review, an approval from someone who | ||
// may not write counts for nothing, and each answer says why. | ||
crates/knotless-run/src/lib.rs+12−1
| @@ -125,7 +125,7 @@ use tokio::sync::{Notify, Semaphore}; | ||
use turns::{Class, Rank, Turns, Whose}; | ||
use workflow::{Superseded, Workflow}; | ||
| ||
const USAGE: &str = "usage: knotless-run [--jetstream <wss://host>] [--executor docker|shell] [--image <image>] [--secrets <file>] [--secrets-key <file>] [--state <file>] [--work <dir>] [--step-timeout <secs>] [--run-timeout <secs>] [--cpus <n>] [--memory <GiB>] [--pids <n>] [--network <name>] [--runtime <name>] [--trusted-runtime <name>] [--trust <did>] [--resolv <file>] [--jobs <n>] [--pull-jobs <n>] [--owner-jobs <n>] [--allowance <minutes>] [--private-allowance <minutes>] [--cache-max <GiB>] [--disk-floor <GiB>] [--caches <dir>] [--paused <file>] [--keep-checks <days>] [--sweep <secs>] [--stop-wait <secs>] [--poll <secs>] [--pulls] [--pulls-only <handle>/<name>] [--unmetered <did>] [--queue [--batch <n>] [--depth 1|2] [--notice <secs>]] [--discover] [<handle>/<name>...]"; | ||
const USAGE: &str = "usage: knotless-run [--jetstream <wss://host>] [--executor docker|shell] [--image <image>] [--secrets <file>] [--secrets-key <file>] [--state <file>] [--work <dir>] [--step-timeout <secs>] [--run-timeout <secs>] [--cpus <n>] [--memory <GiB>] [--pids <n>] [--network <name>] [--runtime <name>] [--trusted-runtime <name>] [--trust <did>] [--resolv <file>] [--jobs <n>] [--pull-jobs <n>] [--owner-jobs <n>] [--allowance <minutes>] [--private-allowance <minutes>] [--cache-max <GiB>] [--disk-floor <GiB>] [--caches <dir>] [--paused <file>] [--keep-checks <days>] [--sweep <secs>] [--stop-wait <secs>] [--poll <secs>] [--pulls] [--pulls-only <handle>/<name>] [--unmetered <did>] [--queue [--batch <n>] [--depth 1|2] [--notice <secs>] [--review-wait <secs>]] [--discover] [<handle>/<name>...]"; | ||
| ||
#[derive(Debug, Clone, PartialEq)] | ||
pub struct Options { | ||
| @@ -217,6 +217,10 @@ pub struct Options { | ||
/// How often the merge queue's records are looked at, besides as | ||
/// Jetstream brings them. | ||
pub notice: Duration, | ||
/// How long a queued pull waits for a review of its head from its | ||
/// branch's gates ([`knotless::approvals::gated`]) before it is | ||
/// ejected, saying so. | ||
pub review_wait: Duration, | ||
/// How often a private repository's space is asked whether anything | ||
/// was written there: Jetstream carries nothing of a space. | ||
pub poll: Duration, | ||
| @@ -269,6 +273,7 @@ impl Options { | ||
batch: 8, | ||
depth: 2, | ||
notice: Duration::from_secs(30), | ||
review_wait: Duration::from_secs(30 * 60), | ||
poll: Duration::from_secs(10), | ||
paused: None, | ||
keep_checks: Some(Duration::from_secs(30 * DAY)), | ||
| @@ -406,6 +411,10 @@ pub fn parse_args(args: &[String]) -> Result<Options, String> { | ||
let secs = count(flag, "seconds", value()?)?; | ||
opts.notice = Duration::from_secs(secs as u64); | ||
} | ||
"--review-wait" => { | ||
let secs = count(flag, "seconds", value()?)?; | ||
opts.review_wait = Duration::from_secs(secs as u64); | ||
} | ||
"--poll" => { | ||
let secs = count(flag, "seconds", value()?)?; | ||
opts.poll = Duration::from_secs(secs as u64); | ||
| @@ -4660,6 +4669,8 @@ mod tests { | ||
); | ||
assert_eq!(opts.notice, Duration::from_secs(30)); | ||
assert_eq!(args("--notice 5 a/b").unwrap().notice.as_secs(), 5); | ||
let wait = args("--review-wait 60 a/b").unwrap().review_wait; | ||
assert_eq!(wait.as_secs(), 60); | ||
assert_eq!(args("a/b").unwrap().poll.as_secs(), 10); | ||
assert_eq!(args("--poll 2 a/b").unwrap().poll.as_secs(), 2); | ||
assert_eq!( | ||
crates/knotless-run/src/queue.rs+47−8
| @@ -57,6 +57,7 @@ use knotless::session::{self, Session}; | ||
use knotless::xrpc::XrpcError; | ||
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; | ||
use std::sync::{Arc, Mutex}; | ||
use std::time::Duration; | ||
| ||
/// Why landing was refused, when the gateway refused it: a runner that | ||
/// lands as a runner the repository names, through the gateway, is refused | ||
| @@ -217,6 +218,15 @@ fn preflight( | ||
stands | ||
} | ||
| ||
/// Whether a pull queued at `since` still waits for its gates' review, as | ||
/// `wait` has not passed since. One whose time it cannot read waits no | ||
/// longer. | ||
fn waits(since: &str, wait: Duration) -> bool { | ||
let queued = humantime::parse_rfc3339_weak(since); | ||
let waited = queued.map(|at| at.elapsed().unwrap_or_default()); | ||
waited.is_ok_and(|waited| waited < wait) | ||
} | ||
| ||
/// Why a pull that conflicts in `files` with the branch `into` itself, at | ||
/// `tip`, is ejected. | ||
fn conflicted(into: &str, tip: &str, files: &[String]) -> String { | ||
| @@ -528,7 +538,9 @@ impl Runner { | ||
/// at its head on `tip`, it waits no longer: it is a workflow the pull | ||
/// takes away, say, and its batch will say what it does. Into a branch | ||
/// whose rule asks for reviews, one not reviewed as it asks is ejected | ||
/// first, as a merge would refuse it. | ||
/// first, as a merge would refuse it; one lacking only a gate's review | ||
/// of its head has its pre-flights run, but is not ready until it has | ||
/// one ([`Runner::reviewed`]). | ||
async fn preflown( | ||
self: &Arc<Self>, | ||
on: &Queue<'_>, | ||
| @@ -536,7 +548,7 @@ impl Runner { | ||
tip: &str, | ||
waiting: Vec<Item>, | ||
) -> Result<HashSet<String>> { | ||
let waiting = self.reviewed(on, line, tip, waiting).await?; | ||
let (waiting, held) = self.reviewed(on, line, tip, waiting).await?; | ||
let refname = format!("refs/heads/{}", on.branch); | ||
let loaded = workflow::load(on.git, tip).await?; | ||
let first: BTreeSet<String> = (loaded.into_iter()) | ||
| @@ -545,7 +557,8 @@ impl Runner { | ||
.map(|w| w.name) | ||
.collect(); | ||
if first.is_empty() { | ||
return Ok(waiting.into_iter().map(|i| i.uri).collect()); | ||
let ready = waiting.into_iter().map(|i| i.uri); | ||
return Ok(ready.filter(|uri| !held.contains(uri)).collect()); | ||
} | ||
let mut ready = HashSet::new(); | ||
let (record, me) = (&on.target.record, &on.me.did); | ||
| @@ -611,22 +624,27 @@ impl Runner { | ||
} | ||
ready.insert(item.uri); | ||
} | ||
ready.retain(|uri| !held.contains(uri)); | ||
Ok(ready) | ||
} | ||
| ||
/// Of `waiting`, those reviewed as their branch's rule asks, on the | ||
/// branch at `tip`; the rest are ejected, saying what they lack. One | ||
/// whose branch is gone goes on, to be ejected as a batch is built. | ||
/// branch at `tip`, and of those the ones held, by URI, as all they | ||
/// lack is a gate's review of their head: queued less than | ||
/// `--review-wait` ago, they wait for it. The rest are ejected, saying | ||
/// what they lack. One whose branch is gone goes on, to be ejected as | ||
/// a batch is built. | ||
async fn reviewed( | ||
&self, | ||
on: &Queue<'_>, | ||
line: &mut Line, | ||
tip: &str, | ||
waiting: Vec<Item>, | ||
) -> Result<Vec<Item>> { | ||
) -> Result<(Vec<Item>, HashSet<String>)> { | ||
let mut held = HashSet::new(); | ||
let rule = on.target.record.rule(on.branch); | ||
if rule.is_none_or(|r| r.reviews.is_none()) { | ||
return Ok(waiting); | ||
return Ok((waiting, held)); | ||
} | ||
let mut reviewed = Vec::new(); | ||
for item in waiting { | ||
| @@ -638,7 +656,20 @@ impl Runner { | ||
let standing = | ||
approvals::of(&self.ctx, on.target.facts(), &item, on.git, tip, &sha).await?; | ||
let url = &on.watch.remote.url; | ||
let since = item.queue.as_ref().map_or("", |q| q.created_at.as_str()); | ||
if standing.as_ref().is_some_and(|s| s.awaits()) && waits(since, self.opts.review_wait) | ||
{ | ||
held.insert(item.uri.clone()); | ||
reviewed.push(item); | ||
continue; | ||
} | ||
let awaited = standing.as_ref().is_some_and(|s| !s.awaiting.is_empty()); | ||
let why = standing.and_then(|s| approvals::refusal(url, &item.uri, on.branch, &s)); | ||
let waited = humantime::format_duration(self.opts.review_wait); | ||
let why = why.map(|why| match awaited { | ||
true => format!("{why}. It waited {waited} for that review"), | ||
false => why, | ||
}); | ||
let eject = why | ||
.as_deref() | ||
.map(|why| self.eject(on, line, &item, Some(&sha), why, &[])); | ||
| @@ -646,7 +677,7 @@ impl Runner { | ||
let ejected = ejected.transpose()?; | ||
reviewed.extend(ejected.is_none().then_some(item)); | ||
} | ||
Ok(reviewed) | ||
Ok((reviewed, held)) | ||
} | ||
| ||
/// Runs the pre-flights `only` of `item`, at its `head` (and the | ||
| @@ -932,6 +963,14 @@ mod tests { | ||
assert!(preflight(&first(&["a"]), &runs, "tip", later, "did:ci").running); | ||
} | ||
| ||
#[test] | ||
fn a_pull_waits_for_its_gates_a_while() { | ||
let wait = Duration::from_secs(60); | ||
assert!(waits(&knotless::records::now(), wait)); | ||
assert!(!waits("2026-01-01T00:00:00.000Z", wait)); | ||
assert!(!waits("", wait)); | ||
} | ||
| ||
#[test] | ||
fn a_conflict_with_the_branch_itself_says_so() { | ||
let why = conflicted("main", &"a".repeat(40), &["f".into(), "g".into()]); | ||