~ knotless Search/
↑↓ move↵ open⇧↵ all results

chadtmiller.com/knotless
Switch repository
Loading…

git that lives in your Atmosphere account

knotless.dev
1 collaborator

knotless 0.13.0 Latest 2 packages, 1 image

1
10

A review agent reads a pull again when its body answers a request for changes

Merged chadtmiller.com asked to merge into main from chadtmiller.com/knotless:agent/rereview-body
.changeset/review-reads-edited-body.mdnew+7−0
View file
@@ -0,0 +1,7 @@
---
default: patch
---
# A review agent reads a pull again when its body answers a request for changes
After `knotless-agent review` asks for changes, editing the pull's title or body has it review the pull again at the same head, as one of its rounds, so a request about what the pull says no longer needs a push to clear. `knotless-claude-review` also no longer asks for changes over what a required check already fails the pull for.
crates/knotless-agent/src/lib.rs+37−9
View file
@@ -28,6 +28,9 @@
//! 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.
//! 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
@@ -169,8 +172,15 @@ struct Earlier {
body: String,
/// The head it looked at.
commit: Option<String>,
/// When it was written.
#[serde(skip)]
at: String,
}
/// What a review read of a pull: its head, and when its title or body
/// had last changed.
type Read = (String, Option<String>);
/// What the command answers: the review to write.
#[derive(Deserialize)]
struct Answer {
@@ -194,9 +204,10 @@ struct Reviewer {
owned: Mutex<BTreeMap<String, Owned>>,
/// Those whose pulls ask it for a review: a ref they push may move one.
authors: Mutex<BTreeSet<String>>,
/// The head it last reviewed each pull at, by the pull: the appview
/// may not have heard of the review yet as it reads the pull again.
posted: Mutex<BTreeMap<String, String>>,
/// The head it last reviewed each pull at, and when the pull's title
/// or body had last changed then, by the pull: the appview may not
/// have heard of the review yet as it reads the pull again.
posted: Mutex<BTreeMap<String, Read>>,
door: Notify,
}
@@ -341,15 +352,24 @@ impl Reviewer {
let Some(commit) = self.head(head).await? else {
return Ok(None);
};
let posted = self.posted.lock().unwrap().get(&pull.uri) == Some(&commit);
let mine = |r: &knotless::reviews::Review| r.by == self.me && r.at_head(&commit);
if posted || pull.reviews.iter().any(mine) {
let edited = pull.updated_at.as_deref();
let read: Read = (commit.clone(), pull.updated_at.clone());
let posted = self.posted.lock().unwrap().get(&pull.uri) == Some(&read);
// Its verdict at this head stands, unless it asked for changes and
// the title or body may since have answered them: the thread says.
let stands = |r: &knotless::reviews::Review| {
r.by == self.me
&& r.at_head(&commit)
&& (r.verdict != knotless::reviews::CHANGES || edited.is_none())
};
if posted || pull.reviews.iter().any(stands) {
return Ok(None);
}
let thread = forge::show(&self.ctx, url, &pull.uri).await?;
let mut earlier = earlier(&thread.comments, &self.me);
// The thread may know of a review at this head the pull does not yet.
let seen = (earlier.iter()).any(|e| e.commit.as_deref() == Some(commit.as_str()));
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.
if let Some(last) = earlier.last()
&& let Some(at) = last.commit.as_deref().filter(|at| *at != commit)
@@ -362,7 +382,7 @@ impl Reviewer {
);
let verdict = last.verdict.clone();
forge::comment(&self.ctx, url, &pull.uri, &body, Some(&verdict), &[], &[]).await?;
self.posted.lock().unwrap().insert(pull.uri.clone(), commit);
self.posted.lock().unwrap().insert(pull.uri.clone(), read);
return Ok(Some(verdict));
}
earlier.retain(|e| !e.body.starts_with(CARRIED));
@@ -395,7 +415,7 @@ impl Reviewer {
}
let (body, verdict) = (&answer.body, Some(answer.verdict.as_str()));
forge::comment(&self.ctx, url, &pull.uri, body, verdict, &[], &answer.lines).await?;
self.posted.lock().unwrap().insert(pull.uri.clone(), commit);
self.posted.lock().unwrap().insert(pull.uri.clone(), read);
Ok(Some(answer.verdict))
}
@@ -462,6 +482,13 @@ impl Reviewer {
}
}
/// Whether `review` asked for changes and the pull's title or body
/// changed after it, at `edited`: both are written as RFC 3339 in UTC to
/// the millisecond, so the later is the greater.
fn answered(review: &Earlier, edited: Option<&str>) -> bool {
review.verdict == knotless::reviews::CHANGES && edited.is_some_and(|at| at > review.at.as_str())
}
/// The reviews `me` wrote among `comments`, oldest first.
fn earlier(comments: &[forge::Entry], me: &str) -> Vec<Earlier> {
let mine = comments.iter().filter(|c| c.author == me);
@@ -473,6 +500,7 @@ fn earlier(comments: &[forge::Entry], me: &str) -> Vec<Earlier> {
verdict,
body: c.body,
commit: c.commit,
at: c.created_at,
})
});
reviews.collect()
crates/knotless-agent/tests/review.rs+36−0
View file
@@ -371,6 +371,42 @@ 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 then changes what it says is
// read again at the same head, as a round; once approved, an edit is
// read no more.
let answered = pull("answered", "Answered", &asking);
wait_until(|| {
owner
.cli(&["list", &url], b"")
.1
.contains(answered.rsplit('/').next().unwrap())
});
once(&opts);
assert_eq!(asked(&bot, "Answered"), 1);
let head = given(&bot, "Answered")["head"]["commit"].clone();
wait_until(|| ok(&owner, &["show", &url, &answered]).contains("changes"));
once(&opts);
assert_eq!(asked(&bot, "Answered"), 1);
ok(
&owner,
&["edit", &url, &answered, "-m", "The file is named here."],
);
wait_until(|| ok(&owner, &["show", &url, &answered]).contains("named here"));
once(&opts);
assert_eq!(asked(&bot, "Answered"), 2);
let given_answered = given(&bot, "Answered");
assert_eq!(given_answered["round"], 2);
assert_eq!(given_answered["head"]["commit"], head);
assert_eq!(given_answered["body"], "The file is named here.");
assert_eq!(given_answered["earlier"][0]["commit"], head);
let (said, _) = reviews(&bot, &answered);
assert_eq!(said[1]["verdict"], "approve");
wait_until(|| ok(&owner, &["show", &url, &answered]).contains("approve"));
ok(&owner, &["edit", &url, &answered, "-m", "Said again."]);
wait_until(|| ok(&owner, &["show", &url, &answered]).contains("Said again"));
once(&opts);
assert_eq!(asked(&bot, "Answered"), 2);
// One pushed to as it is read is read again at its new head.
let moving = pull("moving", "Moving", &asking);
wait_until(|| {
crates/knotless-agent/claude-review+2−0
View file
@@ -74,7 +74,9 @@ How to review:
- Read the diff with pull_diff, and whatever files around it you need to judge it.
- Look for what would make the change wrong: bugs, broken edge cases, security holes, missing tests, docs or other files the repository asks to change with it. Read the checks if they say something failed.
- Say little and be sure of it. Leave out style the repository's tools already check, and anything you are guessing at.
- 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.
Answer with:
- verdict: "changes" when something must change before it merges, else "approve".
docs/design/collaboration.md+5−1
View file
@@ -96,7 +96,11 @@ 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. What it reviewed it reads back
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 not read again for an edit. What it reviewed it reads back
from its own records, so it keeps nothing else.
Each review spends its operator's model usage, and anyone may ask any
docs/guide/reviews.md+3−1
View file
@@ -106,6 +106,8 @@ The reviewer listens to Jetstream, a public stream of every account's record cha
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.
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.
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.
If the head moves while Claude is reading the pull, the agent drops the answer and reviews the pull at its new head.
@@ -120,7 +122,7 @@ The reviewer's account may not write to your repository, so its verdict is advic
knotless protect knotless://alice.example.com/app main --queue --gate reviewer.example.com
```
`protect` replaces the branch's rule, so give it the flags the rule has now as well; `knotless rules <url>` lists them. From then on, a pull into `main` waits for the reviewer's verdict at its head. The merge queue holds it meanwhile, running its checks at the same time. A request for changes blocks it, and queueing it says so: `changes asked by @reviewer.example.com`. Push a fix and the reviewer reads it again.
`protect` replaces the branch's rule, so give it the flags the rule has now as well; `knotless rules <url>` lists them. From then on, a pull into `main` waits for the reviewer's verdict at its head. The merge queue holds it meanwhile, running its checks at the same time. A request for changes blocks it, and queueing it says so: `changes asked by @reviewer.example.com`. Push a fix, or edit the body when that is what it asked about, and the reviewer reads it again.
When the reviewer is wrong, or has used up its `--rounds` on a pull, approve the pull's head yourself, as the owner or a collaborator: your approval, given after its review, stands in for it. A pull it has not reviewed within half an hour is ejected from the queue, saying so, so a reviewer that is down holds pulls no longer than that. [Gates](../design/collaboration.md#gates) says how they are counted.