chadtmiller.com/knotless
git that lives in your Atmosphere account
1
10
A branch's gates are its required reviewers, asked of every pull into it
.changeset/required-reviewers.mdnew+7−0
| @@ -0,0 +1,7 @@ | ||
--- | ||
default: minor | ||
--- | ||
| ||
# A branch's gates are its required reviewers | ||
| ||
Every pull into a protected branch now asks the gates its rule names to review it, so they show among its reviewers before they review, marked "Required reviewer for main" on its page. `unask-review` refuses to stop asking one: change the rule instead. | ||
crates/knotless/src/approvals.rs+14−1
| @@ -195,7 +195,12 @@ pub fn stand( | ||
let approving: Vec<&Review> = standing() | ||
.filter(|r| r.verdict == reviews::APPROVE && why(r).is_none()) | ||
.collect(); | ||
let not_counted = standing().filter_map(|r| Some((r.by.clone(), why(r)?))); | ||
// A required reviewer's approval does what it is for, so it is not | ||
// said not to count. | ||
let gate = |r: &&Review| rule.gates.iter().any(|g| g.did == r.by); | ||
let not_counted = standing() | ||
.filter(|r| !gate(r)) | ||
.filter_map(|r| Some((r.by.clone(), why(r)?))); | ||
let mut unowned: Vec<(String, Vec<String>)> = Vec::new(); | ||
let mut authored = Vec::new(); | ||
let file = owners.filter(|_| rule.code_owners); | ||
| @@ -431,6 +436,14 @@ mod tests { | ||
// 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); | ||
// A stranger's approval is said not to count; the gate's is not. | ||
let both = [ | ||
review("did:bot", reviews::APPROVE, "head"), | ||
review("did:x", reviews::APPROVE, "head"), | ||
]; | ||
let uncounted = s(&both, &rule).uncounted; | ||
let by: Vec<&str> = uncounted.iter().map(|(by, _)| by.as_str()).collect(); | ||
assert_eq!(by, ["did:x"]); | ||
// Its changes block, at any head, and are no wait. | ||
let asked = [review("did:bot", reviews::CHANGES, "old")]; | ||
let blocked = s(&asked, &rule); | ||
crates/knotless/src/forge.rs+15−2
| @@ -165,7 +165,8 @@ pub struct Item { | ||
pub draft: bool, | ||
/// Those asked to review a pull, by DID: first those its author asks, | ||
/// then those its repository's owner or collaborators ask | ||
/// ([`assigned`]). | ||
/// ([`assigned`]), then the required reviewers the rule protecting its | ||
/// base names, its gates ([`RepoRecord::required_reviewers`]). | ||
#[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
pub reviewers: Vec<String>, | ||
/// Those its repository's owner or collaborators assign it to, by | ||
| @@ -471,7 +472,8 @@ pub fn assemble(repo: &RepoRecord, owner: &str, entries: &[Entry]) -> Vec<Item> | ||
item.assignees = assigned(repo, owner, &assignments, &item.uri, Role::Assignee, now); | ||
if item.kind == "pull" { | ||
let asked = assigned(repo, owner, &assignments, &item.uri, Role::Reviewer, now); | ||
for did in asked { | ||
let required = repo.required_reviewers(item.base.as_deref().unwrap_or_default()); | ||
for did in asked.into_iter().chain(required.map(str::to_string)) { | ||
if did != item.author && !item.reviewers.contains(&did) { | ||
item.reviewers.push(did); | ||
} | ||
| @@ -2111,6 +2113,17 @@ pub async fn assign( | ||
} | ||
let entries = freshened(entries, fresh); | ||
let item = thread(&t.record, owner, &entries, subject)?.item; | ||
let base = item.base.as_deref().unwrap_or_default(); | ||
let required: Vec<&str> = t.record.required_reviewers(base).collect(); | ||
let gate = names | ||
.iter() | ||
.zip(&dids) | ||
.find(|(_, d)| required.contains(&d.as_str())); | ||
if let (true, Role::Reviewer, Some((name, _))) = (removed, role, gate) { | ||
bail!( | ||
"{name} is a required reviewer for {base}: its rule asks every pull, so change the rule to stop" | ||
); | ||
} | ||
let still = dids.iter().find(|d| item.reviewers.contains(d)); | ||
if let (true, Role::Reviewer, Some(did)) = (removed, role, still) { | ||
let asker = match by_author { | ||
crates/knotless/src/records.rs+9−0
| @@ -257,6 +257,15 @@ impl RepoRecord { | ||
self.rules.iter().find(|r| r.branch == branch) | ||
} | ||
| ||
/// The accounts every pull into `branch` asks to review it and waits | ||
/// for: the gates of the rule protecting it, by DID. | ||
pub fn required_reviewers(&self, branch: &str) -> impl Iterator<Item = &str> { | ||
let reviews = self.rule(branch).and_then(|r| r.reviews.as_ref()); | ||
reviews | ||
.into_iter() | ||
.flat_map(|r| r.gates.iter().map(|g| g.did.as_str())) | ||
} | ||
| ||
/// Whether pulls into `branch` land only through the merge queue. | ||
pub fn queue_only(&self, branch: &str) -> bool { | ||
self.rule(branch).is_some_and(|r| r.queue) | ||
crates/knotless-serve/src/web/views.rs+10−1
| @@ -259,9 +259,13 @@ pub struct ItemView { | ||
#[serde(skip_serializing_if = "std::ops::Not::not")] | ||
pub draft: bool, | ||
/// Those asked to review a pull: by its author, then by the owner or | ||
/// a collaborator. | ||
/// a collaborator, then the required reviewers its base's rule names. | ||
#[serde(skip_serializing_if = "Vec::is_empty")] | ||
pub reviewers: Vec<ActorView>, | ||
/// Of `reviewers`, those its base's rule requires: asked of every | ||
/// pull into it, and stopped only by changing the rule. | ||
#[serde(skip_serializing_if = "Vec::is_empty")] | ||
pub required_reviewers: Vec<ActorView>, | ||
/// Those the owner or a collaborator assigns it to. | ||
#[serde(skip_serializing_if = "Vec::is_empty")] | ||
pub assignees: Vec<ActorView>, | ||
| @@ -273,6 +277,10 @@ pub struct ItemView { | ||
pub async fn item_view(app: &App, record: &RepoRecord, item: &Item) -> ItemView { | ||
let labels = item.labels.iter().filter_map(|name| record.label(name)); | ||
let labels = labels.map(label_view); | ||
let base = item.base.as_deref().unwrap_or_default(); | ||
let required = record | ||
.required_reviewers(base) | ||
.filter(|r| item.reviewers.iter().any(|d| d == r)); | ||
ItemView { | ||
uri: item.uri.clone(), | ||
kind: item.kind.clone(), | ||
| @@ -284,6 +292,7 @@ pub async fn item_view(app: &App, record: &RepoRecord, item: &Item) -> ItemView | ||
comments: item.comments, | ||
draft: item.draft, | ||
reviewers: futures::future::join_all(item.reviewers.iter().map(|r| actor(app, r))).await, | ||
required_reviewers: futures::future::join_all(required.map(|r| actor(app, r))).await, | ||
assignees: futures::future::join_all(item.assignees.iter().map(|a| actor(app, a))).await, | ||
labels: labels.collect(), | ||
} | ||
crates/knotless-serve/tests/forge.rs+31−0
| @@ -2376,6 +2376,37 @@ fn maintainers_dismiss_reviews() { | ||
// would not count. | ||
let pull = ok(&owner, &["pull", &url, "main", &url, "a", "Mine"]); | ||
let issue = ok(&owner, &["issue", &url, "Broken"]); | ||
// The gate is asked of it as its required reviewer, and stays asked. | ||
let shown = ok(&collab, &["show", &url, &pull]); | ||
assert!( | ||
shown.contains(&format!("review requested: {gate}")), | ||
"{shown}" | ||
); | ||
refused( | ||
&owner, | ||
&["unask-review", &url, &pull, gate], | ||
"is a required reviewer for main", | ||
); | ||
// The pages mark it so. | ||
let listed = reqwest::Url::parse_with_params( | ||
&format!("{}/xrpc/dev.knotless.listItems", av.base), | ||
[ | ||
( | ||
"repo", | ||
format!("at://{}/dev.knotless.repo/dismissed", owner.account.did).as_str(), | ||
), | ||
("kind", "pull"), | ||
], | ||
); | ||
let listed = listed.unwrap(); | ||
let required = || -> Value { | ||
let page: Value = reqwest::blocking::get(listed.clone()) | ||
.unwrap() | ||
.json() | ||
.unwrap(); | ||
page["items"][0]["requiredReviewers"][0]["did"].clone() | ||
}; | ||
wait_until(|| required() == bot.account.did.as_str()); | ||
ok(&bot, &["comment", &url, &pull, "Not yet", "--changes"]); | ||
let blocked = |who: &Sandbox| { | ||
let checked = ok(who, &["merge", &url, &pull, "--check"]); | ||
docs/design/collaboration.md+4−1
| @@ -669,7 +669,10 @@ past it. | ||
A review agent may not write to the repository, so its reviews count | ||
toward nothing above. A rule's `gates` (`knotless protect --gate | ||
<handle>`) name accounts whose reviews hold the pull anyway. Each gate's | ||
newest verdict stands: | ||
newest verdict stands. Each gate is a required reviewer: every pull into | ||
the branch asks it for a review, after those its author and maintainers | ||
ask, and the pages mark it so. It stays asked until the rule stops | ||
naming it, so `knotless unask-review` refuses it. | ||
| ||
- With none yet, or one of an earlier head, the pull waits. Merging | ||
refuses it, and the merge queue holds it until the gate reviews its | ||
docs/reference/api.md+1−1
| @@ -571,7 +571,7 @@ name it has now. | ||
| `dev.knotless.getCommit?repo=&sha=` | a commit and its diff | | ||
| `dev.knotless.listBranches?repo=&limit=&cursor=&commits=` | the branches, the default first, 100 a page by default; with `commits`, each with its tip and that commit's newest checks | | ||
| `dev.knotless.listTags?repo=&limit=&cursor=&commits=` | the tags that name a commit, the newest commit first, 100 a page by default; with `commits`, each with its commit and that commit's newest checks | | ||
| `dev.knotless.listItems?repo=&kind=&state=&q=&limit=&cursor=` | the open or closed pulls or issues that the filter `q` matches ([collaboration.md](../design/collaboration.md#filtering-pulls-and-issues)), `review-requested:@me` and `assignee:@me` the asker, each with `draft` for a draft pull, the `reviewers` asked and the `assignees`, 50 a page by default, how many of each, and the repository's labels and the items' authors, for a filter to name | | ||
| `dev.knotless.listItems?repo=&kind=&state=&q=&limit=&cursor=` | the open or closed pulls or issues that the filter `q` matches ([collaboration.md](../design/collaboration.md#filtering-pulls-and-issues)), `review-requested:@me` and `assignee:@me` the asker, each with `draft` for a draft pull, the `reviewers` asked, its base's required reviewers among them and named again in `requiredReviewers`, and the `assignees`, 50 a page by default, how many of each, and the repository's labels and the items' authors, for a filter to name | | ||
| `dev.knotless.getThread?repo=&uri=&filesOnly=&firstRows=` | a pull or an issue and everything said on it, each comment with its `uri`, Markdown `body` and, for a review, the `commit` it looked at; on a pull's lines, its `line` (`path`, `side`, `line`, `startLine`, the `lines` it names as they were, at most 20, and `outdated` once that side of the file has changed), the `review` it went with and `resolved`; a dismissal's `dismisses`, the review it dismissed; an answer's `reply`, the comment starting its thread; for a pull, `reviews`, where each reviewer stands (`outdated` once the open pull has moved past the head it looked at, `maintainer` for the owner or a collaborator, `gate` for an account the rule protecting its base waits for a review from, `dismissed` once the owner or a collaborator dismissed it), and in `changes` the `head` it is at; for a merged pull, `revertedBy` (`{uri, title}`), the revert whose merge undid it; for an issue, `mentionedBy`, the pulls on the repository whose title or body names it, oldest first, each its `item` and `closes` where it names it after a closing keyword; in each `html`, a bare `at://` URI of a pull or issue on the repository, or a link to one, as a link to its page, a bare one showing its title; for a pull of a public repository, the minutes each runner counts for its owner (`dev.knotless.minutes`); with an index, `reactions` on the item and on each comment, by content in GitHub's order, each naming the asker's own `reaction` when it has an asker; and with an asker, what it may do (`viewer`: `setState`, `hide`, `revert` for a merged pull, `assign`, `dismiss` for a pull, and the `labels` it may set); with `filesOnly`, a pull's changed files without their rows, and with `firstRows` too, the rows of the first files, up to 10 files or 1500 lines, passing over any that changes more than 400 | | ||
| `dev.knotless.getPullDiff?repo=&uri=&paths=` | the files of a pull's diff that `paths` names (repeated, 1 to 50), rows and all: what the pull page fetches as the reader comes to each file | | ||
| `dev.knotless.getTemplate?repo=&kind=&base=` | the template a new pull (at `base`) or issue is asked to follow, `.knotless/<kind>.md`, as `knotless template` reads it; `{}` with none, 400 for a `kind` that is neither | | ||
docs/reference/cli.md+3−3
| @@ -148,7 +148,7 @@ moved, edits are made again in the file as it is there. | ||
| `close <url> <at-uri>` / `reopen <url> <at-uri>` | change an item's state | | ||
| `label <url> <at-uri> <label>...` / `unlabel <url> <at-uri> <label>...` | as owner, collaborator or a triager of their scope, put labels on an item or take them off; prints those it carries now | | ||
| `assign <url> <at-uri> <handle>...` / `unassign <url> <at-uri> <handle>...` | as owner or collaborator, assign a pull or issue to accounts, or take them off it; prints those it is assigned to now ([collaboration.md](../design/collaboration.md#assignees)) | | ||
| `ask-review <url> <at-uri> <handle>...` / `unask-review <url> <at-uri> <handle>...` | ask accounts to review a pull, or stop asking them: as its author, in the pull, as `edit --reviewer` does, or as owner or collaborator, in your own record. Each stops asking only those it asked, and is told who asked when it did not. Prints those asked now | | ||
| `ask-review <url> <at-uri> <handle>...` / `unask-review <url> <at-uri> <handle>...` | ask accounts to review a pull, or stop asking them: as its author, in the pull, as `edit --reviewer` does, or as owner or collaborator, in your own record. Each stops asking only those it asked, and is told who asked when it did not. A required reviewer, a gate of the rule protecting the pull's base, is asked of every pull and stays asked until the rule stops naming it, so unasking one fails Prints those asked now | | ||
docs/reference/cli.mdline 151Missing a full stop before "Prints those asked now". | ||
| `dismiss <url> <at-uri> <handle> <reason>` | as owner or collaborator, dismiss that reviewer's standing review of a pull, saying why (`-` reads the reason from stdin): a comment naming it, so it counts for nothing, for or against, until they review again. Prints the comment's URI; `show` then marks the review `, dismissed` | | ||
| `labels <url>` | the labels a repository defines: name, color, `exclusive` and description, tab-separated | | ||
| `labels <url> add <name> [--color <#hex>] [--description <text>] [--exclusive]` / `labels <url> remove <name>` | as owner, define a label (or redefine it), or take one away, off everything it is on | | ||
| @@ -170,7 +170,7 @@ What they print: | ||
| `comment` | the `at://` URI of each comment it wrote, a line each: the review first, then its comments on lines | | ||
| `resolve`, `unresolve` | `resolved`, `unresolved` | | ||
| `list` | a line per item: `<kind>`, `<state>`, `<uri>`, `<title>`, tab-separated | | ||
| `show` | `<state> <kind> by <author>: <title>`, then `draft` for a draft pull, `review requested: <handles>` for one that asks for reviews, then `closed by <pull>` for an issue a pull closed, `mentioned in <pull>` for each pull whose title or body names an issue, oldest first, ending `, to close it` where it names it after a closing keyword, `reverts <pull>` for a revert, `reverted by <pull>` for a merged pull whose revert merged, then for a pull each reviewer's standing review as `review: <reviewer> <verdict> at <commit>` (the first 12 characters of the head it looked at), then for an open pull in or ejected from the merge queue `merge queue: <state> at <createdAt>` and where it stands, and for one ejected as checks failed `logs: <command>`, the `knotless checks` that reads their logs, then the body and each comment as `<author> [<verdict>]: <body>`, where a comment on lines says ` on <path>:<lines>[:old] (<uri>[, resolved])` before the colon, and an answer in a thread ` answering <uri>` | | ||
| `show` | `<state> <kind> by <author>: <title>`, then `draft` for a draft pull, `review requested: <handles>` for one that asks for reviews, its base's required reviewers among them, then `closed by <pull>` for an issue a pull closed, `mentioned in <pull>` for each pull whose title or body names an issue, oldest first, ending `, to close it` where it names it after a closing keyword, `reverts <pull>` for a revert, `reverted by <pull>` for a merged pull whose revert merged, then for a pull each reviewer's standing review as `review: <reviewer> <verdict> at <commit>` (the first 12 characters of the head it looked at), then for an open pull in or ejected from the merge queue `merge queue: <state> at <createdAt>` and where it stands, and for one ejected as checks failed `logs: <command>`, the `knotless checks` that reads their logs, then the body and each comment as `<author> [<verdict>]: <body>`, where a comment on lines says ` on <path>:<lines>[:old] (<uri>[, resolved])` before the colon, and an answer in a thread ` answering <uri>` | | ||
| `merge` | the commit the branch is now at, then `deleted <branch>` and a `closed <issue>` line per issue it closed; queued instead, `<base> takes pulls through the merge queue: queued, <n> of <of>`, then where to follow it | | ||
| `merge --queue` | `queued into <base>: <n> of <of>`; queued again at the head the queue last ejected, a line saying so and why; then where to follow it, and to read an ejection's logs before queueing again | | ||
| `merge --check` | `<state>`, `<method>` and `checks <checks>`, and `reviews <passed or blocked>` when its branch asks for reviews, tab-separated, then a `missing <what>` line for each thing its reviews lack (`reviews 1 of 2`, `owners of web/: @alice or @bob`, `changes asked by @carol`, `a review of its head from @bot`; owners who are the pull's author alone add `, its author alone, whose own approval never counts, so web/ needs another owner in CODEOWNERS`), an `uncounted <why>` line for each approval that does not count (`@dan's approval does not count: only the owner's and collaborators' approvals count`), a `warning <what>` line for each CODEOWNERS line skipped, a `conflict <path>` line per file that conflicts, `deletes <branch>` for the branch it would delete, `may <what>`, `queue only` when its branch takes pulls only through the merge queue, and last why it cannot merge, in the words `merge` would refuse with, and that the caller may not merge it when it may not | | ||
| @@ -611,7 +611,7 @@ does. | ||
| ||
It reviews a pull that is open and not a draft, that its owner, a | ||
collaborator or a member of the owner's organization opened, and whose | ||
`reviewers` names its account (or any, with `--all`), once at each head | ||
`reviewers` names its account (or any, with `--all`), as it does of every pull into a branch whose rule names it a gate, once at each head | ||
commit, up to `--rounds` times. It never reviews its own. A head whose | ||
changes against the base are those of the head it last reviewed, as after | ||
a merge of the base into the branch, is not read again: its last verdict | ||
harness/e2e/flows.spec.mjs+2−2
| @@ -547,8 +547,8 @@ test('naming and removing a gate of a branch', async () => { | ||
const row = (page) => page.locator('li', { has: page.getByRole('link', { name: 'gated', exact: true }) }) | ||
await row(page).getByRole('button', { name: 'Protect' }).click() | ||
await expect(row(page)).toContainText('protected') | ||
await row(page).getByLabel('Wait for a review from').fill('bob.test') | ||
await row(page).locator('form', { has: page.getByLabel('Wait for a review from') }).getByRole('button', { name: 'Add' }).click() | ||
await row(page).getByLabel('Required reviewers').fill('bob.test') | ||
await row(page).locator('form', { has: page.getByLabel('Required reviewers') }).getByRole('button', { name: 'Add' }).click() | ||
const gate = (page) => row(page).locator('.gates li', { hasText: 'bob.test' }) | ||
await expect(gate(page)).toBeVisible() | ||
await anew(page, (expect) => expect(gate(page)).toBeVisible()) | ||