chadtmiller.com/knotless
git that lives in your Atmosphere account
1
10
A review agent's rounds count only its requests for changes
.changeset/review-rounds-count-changes.mdnew+7−0
| @@ -0,0 +1,7 @@ | ||
--- | ||
default: minor | ||
--- | ||
| ||
# A review agent's rounds count only its requests for changes | ||
| ||
`knotless-agent review --rounds` now limits how many times it asks for changes on a pull, not how many times it reviews it. A pull it approved is reviewed again at each later push, and the command is told the commit it approved (`approved`), and how many times it has asked for changes (`changes`). A request for changes is no longer carried to a head that only merges the base in when the pull's title or body was edited after it. | ||
crates/knotless-agent/src/lib.rs+40−13
| @@ -10,8 +10,10 @@ | ||
//! What reads the pull is a command, run by `sh -c` (`--command`), such as | ||
//! `knotless-claude-review`, which has Claude Code read it: it reads the | ||
//! pull as JSON on stdin, its `repo`, `pull`, `title`, `body`, `author`, | ||
//! `base` and `head` (with the commit it is at), which `round` of how many | ||
//! (`rounds`) this is, its own `earlier` reviews of the pull, and what | ||
//! `base` and `head` (with the commit it is at), which `round` this is, | ||
//! how many times it may ask for changes (`rounds`) and has (`changes`), | ||
//! the head its last review `approved`, if it did, its own `earlier` | ||
//! reviews of the pull, and what | ||
//! the repository asks of its reviews (`instructions`: its | ||
//! `.knotless/review.md` on the pull's base branch, never the pull's), and | ||
//! writes `{"verdict": "approve" | "changes", "body": ..., "lines": [...]}` | ||
| @@ -25,15 +27,19 @@ | ||
//! every public repository that account holds: listed from its PDS at | ||
//! each read, so one made meanwhile is watched from then on. | ||
//! | ||
//! A pull is reviewed once at each head, at most `--rounds` times, and only | ||
//! one its repository's owner or a collaborator opened: anyone may ask any | ||
//! account for a review, and each spends what the command costs its | ||
//! operator. A head whose own change is the one last reviewed, as when | ||
//! all it adds is a merge of the base into the branch, is not read again: | ||
//! the last verdict is written at it as it stands, and spends no round. One pushed to while it is read is read again at its new head. | ||
//! One whose title or body its author changed after the agent asked for | ||
//! changes at its head is read again there, as a round: what was asked | ||
//! may be answered in what the pull says rather than in its code. | ||
//! A pull is reviewed once at each head, and only one its repository's | ||
//! owner or a collaborator opened: anyone may ask any account for a | ||
//! review, and each spends what the command costs its operator. It stops | ||
//! once it has asked for changes `--rounds` times: that bounds a pull | ||
//! argued back and forth, while one it approved is reviewed at each head | ||
//! it is pushed to after, from the head it approved. A head whose own | ||
//! change is the one last reviewed, as when all it adds is a merge of the | ||
//! base into the branch, is not read again: the last verdict is written | ||
//! at it as it stands. One pushed to while it is read is read again at | ||
//! its new head. One whose title or body its author changed after the | ||
//! agent asked for changes is read again, at that head or a later one, | ||
//! rather than having that verdict carried: what was asked may be | ||
//! answered in what the pull says rather than in its code. | ||
//! One whose review fails is noted and read again at the next sweep. | ||
//! | ||
//! Jetstream is only the doorbell, as it is for knotless-triage: a pull | ||
| @@ -96,7 +102,7 @@ pub struct Options { | ||
pub command: String, | ||
/// Whether it reviews every open pull, not only those asking it. | ||
pub all: bool, | ||
/// How many times at most it reviews one pull. | ||
/// How many times at most it asks for changes of one pull. | ||
pub rounds: usize, | ||
/// How long the command has for a pull. | ||
pub timeout: Duration, | ||
| @@ -187,7 +193,13 @@ struct Asked<'a> { | ||
head: Head<'a>, | ||
/// Which review of the pull this is, from 1. | ||
round: usize, | ||
/// How many times at most it may ask for changes of the pull. | ||
rounds: usize, | ||
/// How many times it has. | ||
changes: usize, | ||
/// The head its last review approved, when it did: what came since is | ||
/// what is new. | ||
approved: Option<String>, | ||
earlier: Vec<Earlier>, | ||
/// What the repository asks of its reviews. | ||
instructions: Option<String>, | ||
| @@ -479,7 +491,10 @@ impl Agent { | ||
let at_head = (earlier.iter()).rfind(|e| e.commit.as_deref() == Some(commit.as_str())); | ||
let seen = at_head.is_some_and(|e| !answered(e, edited)); | ||
// A head bringing nothing of its own since the last review keeps it. | ||
// Not a request for changes its title or body has since answered: | ||
// that is read again, as at its own head. | ||
if let Some(last) = earlier.last() | ||
&& !answered(last, edited) | ||
&& let Some(at) = last.commit.as_deref().filter(|at| *at != commit) | ||
&& self.same_change(url, pull, at, &commit).await? | ||
&& self.head(head).await?.as_ref() == Some(&commit) | ||
| @@ -493,8 +508,18 @@ impl Agent { | ||
self.posted.lock().unwrap().insert(pull.uri.clone(), read); | ||
return Ok(Some(verdict)); | ||
} | ||
// Where its last verdict, carried or not, approved: what is new is | ||
// what came since. | ||
let approved = earlier | ||
.last() | ||
.filter(|e| e.verdict == knotless::reviews::APPROVE); | ||
let approved = approved.and_then(|e| e.commit.clone()); | ||
earlier.retain(|e| !e.body.starts_with(CARRIED)); | ||
if seen || earlier.len() >= self.opts.rounds { | ||
let changes = earlier | ||
.iter() | ||
.filter(|e| e.verdict == knotless::reviews::CHANGES); | ||
let changes = changes.count(); | ||
if seen || changes >= self.opts.rounds { | ||
return Ok(None); | ||
} | ||
let asked = Asked { | ||
| @@ -511,6 +536,8 @@ impl Agent { | ||
}, | ||
round: earlier.len() + 1, | ||
rounds: self.opts.rounds, | ||
changes, | ||
approved, | ||
earlier, | ||
instructions: self.instructions(url, pull.base.as_deref()).await, | ||
}; | ||
crates/knotless-agent/tests/review.rs+70−4
| @@ -121,7 +121,8 @@ fn ok(s: &Sandbox, args: &[&str]) -> String { | ||
} | ||
| ||
/// Reviews by what the pull says: its first round asks for a change on a | ||
/// line, a later one approves; one broken fails, one garbled answers | ||
/// line, a later one approves, and one stubborn asks every round; one | ||
/// broken fails, one garbled answers | ||
/// nothing readable, one slow takes too long, and one moving waits until | ||
/// the test says go. It notes each pull it is asked about. | ||
const REVIEW: &str = r#"input=$(cat) | ||
| @@ -134,6 +135,7 @@ case "$input" in | ||
touch "$ASKED.started" | ||
while [ ! -e "$ASKED.go" ]; do sleep 0.1; done | ||
echo '{"verdict": "approve", "body": "Fine."}' ;; | ||
*Stubborn*) echo '{"verdict": "changes", "body": "Still not."}' ;; | ||
*'"round":1'*) echo '{"verdict": "changes", "body": "Name the file.", "lines": [{"line": {"path": "f", "line": 1}, "body": "Say what f is."}]}' ;; | ||
*) echo '{"verdict": "approve", "body": "Looks right now."}' ;; | ||
esac | ||
| @@ -267,6 +269,8 @@ fn an_agent_reviews_pulls() { | ||
let given_named = given(&bot, "Name it"); | ||
assert_eq!(given_named["round"], 1); | ||
assert_eq!(given_named["rounds"], 2); | ||
assert_eq!(given_named["changes"], 0); | ||
assert_eq!(given_named["approved"], Value::Null); | ||
assert_eq!(given_named["author"], owner.account.handle.as_str()); | ||
assert_eq!(given_named["base"], "main"); | ||
assert_eq!(given_named["head"]["branch"], "named"); | ||
| @@ -309,24 +313,53 @@ fn an_agent_reviews_pulls() { | ||
assert_eq!(asked(&bot, "Broken"), 2); | ||
| ||
// Pushed to, it is reviewed again, as a second round that knows the | ||
// first; pushed to once more, it has had as many as it may. | ||
// first; pushed to once more after it approved, it is reviewed again, | ||
// from the head it approved: an approval spends none of --rounds. | ||
let sha = owner.commit_file(&work, "f", "named, as asked\n"); | ||
owner.git(&work, &["push", "-q", "-f", &url, "HEAD:refs/heads/named"]); | ||
once(&opts); | ||
let given_named = given(&bot, "Name it"); | ||
assert_eq!(given_named["round"], 2); | ||
assert_eq!(given_named["changes"], 1); | ||
assert_eq!(given_named["approved"], Value::Null); | ||
assert_eq!(given_named["head"]["commit"], sha.as_str()); | ||
assert_eq!(given_named["earlier"][0]["body"], "Name the file."); | ||
let (said, _) = reviews(&bot, &named); | ||
assert_eq!(said.len(), 2); | ||
assert_eq!(said[1]["verdict"], "approve"); | ||
owner.commit_file(&work, "f", "named again\n"); | ||
let again = owner.commit_file(&work, "f", "named again\n"); | ||
owner.git(&work, &["push", "-q", "-f", &url, "HEAD:refs/heads/named"]); | ||
wait_until(|| ok(&owner, &["pull", "show", &url, &named]).contains("approve")); | ||
once(&opts); | ||
assert_eq!(asked(&bot, "Name it"), 2); | ||
assert_eq!(asked(&bot, "Name it"), 3); | ||
let given_named = given(&bot, "Name it"); | ||
assert_eq!(given_named["round"], 3); | ||
assert_eq!(given_named["changes"], 1); | ||
assert_eq!(given_named["approved"], sha.as_str()); | ||
assert_eq!(given_named["head"]["commit"], again.as_str()); | ||
owner.git(&work, &["reset", "-q", "--hard", "HEAD~2"]); | ||
| ||
// One it asks changes of at each head stops at --rounds of them. | ||
let stubborn = pull("stubborn", "Stubborn", &asking); | ||
wait_until(|| { | ||
owner | ||
.cli(&["pull", "list", &url], b"") | ||
.1 | ||
.contains(stubborn.rsplit('/').next().unwrap()) | ||
}); | ||
once(&opts); | ||
for n in ["stubborn again\n", "stubborn once more\n"] { | ||
owner.commit_file(&work, "f", n); | ||
owner.git( | ||
&work, | ||
&["push", "-q", "-f", &url, "HEAD:refs/heads/stubborn"], | ||
); | ||
owner.git(&work, &["reset", "-q", "--hard", "HEAD~1"]); | ||
once(&opts); | ||
} | ||
assert_eq!(asked(&bot, "Stubborn"), 2); | ||
assert_eq!(reviews(&bot, &stubborn).0.len(), 2); | ||
| ||
// A head that only merges the base into the branch changes nothing the | ||
// pull does: the last review is carried to it, read by no command and | ||
// counted as no round. A change of its own is then read as round 2. | ||
| @@ -378,6 +411,39 @@ fn an_agent_reviews_pulls() { | ||
assert_eq!(given_carried["earlier"][0]["body"], "Name the file."); | ||
owner.git(&work, &["reset", "-q", "--hard", "HEAD~1"]); | ||
| ||
// Asked for changes, a pull whose author changes what it says and | ||
// then merges the base in is read again at that merge, as a round: the | ||
// request may be answered, so it is not carried. | ||
let edited = pull("edited", "Edited", &asking); | ||
wait_until(|| { | ||
owner | ||
.cli(&["pull", "list", &url], b"") | ||
.1 | ||
.contains(edited.rsplit('/').next().unwrap()) | ||
}); | ||
once(&opts); | ||
wait_until(|| ok(&owner, &["pull", "show", &url, &edited]).contains("changes")); | ||
ok( | ||
&owner, | ||
&["comment", "edit", &url, &edited, "-m", "Answered in here."], | ||
); | ||
wait_until(|| ok(&owner, &["pull", "show", &url, &edited]).contains("Answered in here")); | ||
owner.commit_file(&work, "g", "the base moves on again\n"); | ||
owner.git(&work, &["push", "-q", &url, "main"]); | ||
owner.git(&work, &["fetch", "-q", &url, "edited"]); | ||
owner.git(&work, &["merge", "-q", "--no-edit", "FETCH_HEAD"]); | ||
let merged = owner.git(&work, &["rev-parse", "HEAD"]); | ||
owner.git(&work, &["push", "-q", &url, "HEAD:refs/heads/edited"]); | ||
owner.git(&work, &["reset", "-q", "--hard", "HEAD~1"]); | ||
once(&opts); | ||
assert_eq!(asked(&bot, "Edited"), 2); | ||
let given_edited = given(&bot, "Edited"); | ||
assert_eq!(given_edited["round"], 2); | ||
assert_eq!(given_edited["head"]["commit"], merged.as_str()); | ||
let (said, _) = reviews(&bot, &edited); | ||
assert_eq!(said.len(), 2); | ||
assert_eq!(said[1]["verdict"], "approve"); | ||
| ||
// Asked for changes, a pull whose author then changes what it says is | ||
// read again at the same head, as a round; once approved, an edit is | ||
// read no more. | ||
crates/knotless-agent/claude-review+2−1
| @@ -78,6 +78,7 @@ How to review: | ||
- Ask for changes only for what review alone would catch. What a check the pull must pass already enforces, as a failing test or a line its body must hold, the check fails on: mention it at most, and do not withhold approval for it. | ||
- On a later round, say whether what your earlier reviews asked for was done, and do not ask again for what was. | ||
- An earlier review at the same head commit as now means the author changed the pull's title or body since: judge whether what it says now answers what you asked. | ||
- When "approved" names a commit, your last review approved the pull at it. Read what changed since with repo_diff from that commit to the head, and judge that: what you approved stands unless what came since breaks it. A merge of the base among it brings the base's changes, which are not the pull's: pull_diff shows what the pull itself changes. | ||
crates/knotless-agent/claude-reviewline 81
| ||
| ||
Answer with: | ||
- verdict: "changes" when something must change before it merges, else "approve". | ||
| @@ -105,7 +106,7 @@ try { | ||
JSON.stringify({ ...pull, instructions: undefined }, null, 2), | ||
'```', | ||
'', | ||
`This is round ${pull.round} of at most ${pull.rounds}. "earlier" holds your earlier reviews of it.`, | ||
`This is review ${pull.round} of this pull. "earlier" holds your earlier reviews of it. You have asked for changes ${pull.changes} times, and may ${pull.rounds} times in all: after that you review it no more.`, | ||
].join('\n') | ||
const args = [ | ||
'-p', ask, | ||
docs/design/collaboration.md+10−6
| @@ -111,12 +111,16 @@ that reviews the pulls asking it: a command, | ||
by default Claude Code (`knotless-claude-review`), reads each pull and | ||
answers a verdict, a body and comments on lines, and the agent writes them | ||
as its own review, with the head it read. It reviews a pull again when it | ||
is pushed to, giving the command its earlier reviews, up to `--rounds` | ||
times, and not again at a head it reviewed, or at one whose changes are | ||
those it last reviewed, which keeps its verdict without spending a round. | ||
After asking for changes, it reads the pull again at the same head once | ||
its author changes the title or body, as a round, since what it asked may | ||
be answered in what the pull says, as a line a check reads. An approval | ||
is pushed to, giving the command its earlier reviews, until it has asked | ||
for changes `--rounds` times, and not again at a head it reviewed, or at | ||
one whose changes are those it last reviewed, which keeps its verdict. | ||
Only requests for changes count toward `--rounds`, since the limit is | ||
there to end a pull argued back and forth: a pull it approved is | ||
reviewed at each later head, and the command is given the head it | ||
approved, to read what came since. After asking for changes, it reads | ||
the pull again once its author changes the title or body, at that head | ||
or the next, rather than keeping its verdict, since what it asked may be | ||
answered in what the pull says, as a line a check reads. An approval | ||
is not read again for an edit. What it reviewed it reads back | ||
from its own records, so it keeps nothing else. | ||
| ||
docs/guide/reviews.md+6−4
| @@ -104,11 +104,11 @@ Then start it without `KNOTLESS_PASSWORD`. Claude reads the repository through t | ||
| ||
The reviewer listens to Jetstream, a public stream of every account's record changes. Jetstream only tells it when to look. When a pull record is written about a watched repository, or an account whose pull asks it pushes a ref, it waits `--settle` seconds (5) and reads the repository again. It also reads every repository when it connects to Jetstream, and every `--sweep` seconds (300) whatever happens. | ||
| ||
It reviews a pull once per head commit. When the author pushes again, it reviews the pull again and gives Claude its earlier reviews, so that Claude checks whether its earlier asks were done. It stops after `--rounds` reviews of one pull, 3 unless you set another number. | ||
It reviews a pull once per head commit. When the author pushes again, it reviews the pull again and gives Claude its earlier reviews, so that Claude checks whether its earlier asks were done. It stops once it has asked for changes `--rounds` times on one pull, 3 unless you set another number. Approvals don't count toward that: a pull it approved is reviewed again at each later push, and Claude is told the commit it approved, so it reads what changed since then. | ||
| ||
When it asked for changes and you then edit the pull's title or body, it reviews the pull again at the same head, as one of its rounds, and tells Claude that what the pull says changed. That answers a request about the body, such as a missing `Shots-moved:` line, without a push. Once it approves, editing the body does not ask it again. | ||
When it asked for changes and you then edit the pull's title or body, it reviews the pull again at the same head and tells Claude that what the pull says changed. That answers a request about the body, such as a missing `Shots-moved:` line, without a push. Once it approves, editing the body does not ask it again. | ||
| ||
A push that changes nothing the pull itself does, such as a merge of the base branch into the pull's branch, is not reviewed again. The agent compares the pull's changes against its base at the new head and at the last head it reviewed, ignoring where in each file they fall. If they match, it writes its last verdict again at the new head, in a review that begins "Carried forward:". That review does not count toward `--rounds`, and Claude is not given it as an earlier review. | ||
A push that changes nothing the pull itself does, such as a merge of the base branch into the pull's branch, is not reviewed again. The agent compares the pull's changes against its base at the new head and at the last head it reviewed, ignoring where in each file they fall. If they match, it writes its last verdict again at the new head, in a review that begins "Carried forward:". That review does not count toward `--rounds`, and Claude is not given it as an earlier review. A request for changes is not carried when you edited the title or body after it: the pull is reviewed again at the new head, since the edit may have answered it. | ||
| ||
If the head moves while Claude is reading the pull, the agent drops the answer and reviews the pull at its new head. | ||
| ||
| @@ -193,7 +193,9 @@ On stdin it gets one object: | ||
| `base` | The base branch. | | ||
| `head` | `repo`, `branch` and `commit` of the pull's head. | | ||
| `round` | Which review of this pull this is, from 1. | | ||
| `rounds` | The most reviews of one pull, from `--rounds`. | | ||
| `rounds` | The most times it asks for changes on one pull, from `--rounds`. | | ||
| `changes` | How many times it has asked for changes on this pull. | | ||
| `approved` | The head commit its last review approved, if it did, else `null`. | | ||
| `earlier` | Its earlier reviews of the pull, each with `verdict`, `body` and `commit`. | | ||
| `instructions` | What `.knotless/review.md` says on the base branch, if it is there. | | ||
| ||
docs/reference/cli.md+7−4
| @@ -763,7 +763,7 @@ does. | ||
| --- | --- | --- | | ||
| `--command <command>` | | the command run with `sh -c` once per review, as `knotless-claude-review` | | ||
| `--all` | off | review every open pull that is not a draft, asked or not | | ||
| `--rounds <n>` | 3 | the most reviews of one pull, one at each head it is pushed to | | ||
| `--rounds <n>` | 3 | the most times it asks for changes on one pull; it stops reviewing the pull after that. Approvals do not count | | ||
| `--timeout <secs>` | 1200 | how long the command has for a pull before it is killed | | ||
| `--jetstream <wss://host>` | `wss://jetstream2.us-east.bsky.network` | where events come from | | ||
| `--sweep <secs>` | 300 | how often every repository is read, whatever events say | | ||
| @@ -774,17 +774,20 @@ 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`), 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 | ||
commit, until it has asked for changes `--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 | ||
is written at the new head, beginning `Carried forward:`, and counts as no | ||
round. | ||
round. A request for changes is not carried so when the pull's title or | ||
body changed after it: the new head is read. | ||
| ||
The command reads one pull as JSON on standard input: `repo` | ||
(`knotless://<owner>/<name>`), `pull` (its AT-URI), `title`, `body`, | ||
`author` (a handle, or a DID where none resolves), `base` (the branch it | ||
merges into), `head` (`repo`, `branch` and the `commit` it is at), `round` | ||
(from 1), `rounds`, `earlier`, its own earlier reviews of the pull, | ||
(from 1), `rounds`, `changes` (how many times it has asked for changes | ||
on the pull), `approved` (the head its last review approved, or | ||
`null`), `earlier`, its own earlier reviews of the pull, | ||
each `verdict`, `body` and `commit`, and `instructions`, what | ||
`.knotless/review.md` says on the pull's base branch, when it is there. It writes `{"verdict": "approve" | | ||
"changes", "body": "...", "lines": [...]}` on standard output, each of | ||