chadtmiller.com/knotless
git that lives in your Atmosphere account
1
10
knotless-agent: a merge of the base into a pull spends no review round
.knotless/workflows/deploy.yml+2−1
| @@ -63,7 +63,8 @@ env: | ||
# open pull by the owner or a collaborator, asked or not (--all), in | ||
# every repository the owner holds, and grain.social's (--owner). | ||
KNOTLESS_REVIEW_REPOS: --owner chadtmiller.com --owner grain.social | ||
KNOTLESS_REVIEW_ARGS: --all | ||
# Five rounds, where a merge of the base into a pull spends none. | ||
KNOTLESS_REVIEW_ARGS: --all --rounds 5 | ||
steps: | ||
- name: tools | ||
run: | | ||
crates/knotless-agent/src/lib.rs+62−3
| @@ -25,7 +25,9 @@ | ||
//! 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. One pushed to while it is read is read again at its new head. | ||
//! 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 review fails is noted and read again at the next sweep. | ||
//! | ||
//! Jetstream is only the doorbell, as it is for knotless-triage: a pull | ||
| @@ -55,6 +57,10 @@ use tokio::sync::Notify; | ||
/// What a repository asks of its reviews, on the branch a pull merges into. | ||
const REVIEW_MD: &str = ".knotless/review.md"; | ||
| ||
/// How a review carried to a new head begins: one that read nothing, so | ||
/// counted as no round and given to the command as no earlier review. | ||
const CARRIED: &str = "Carried forward:"; | ||
| ||
const USAGE: &str = "usage: knotless-agent review --command <command> [--all] [--rounds <n>] [--timeout <secs>] [--jetstream <wss://host>] [--sweep <secs>] [--settle <secs>] [--once] [--owner <handle|did>]... [<handle>/<name>...]"; | ||
| ||
#[derive(Clone)] | ||
| @@ -341,8 +347,26 @@ impl Reviewer { | ||
return Ok(None); | ||
} | ||
let thread = forge::show(&self.ctx, url, &pull.uri).await?; | ||
let earlier = earlier(&thread.comments, &self.me); | ||
if earlier.len() >= self.opts.rounds { | ||
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())); | ||
// 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) | ||
&& self.same_change(url, pull, at, &commit).await? | ||
&& self.head(head).await?.as_ref() == Some(&commit) | ||
{ | ||
let body = format!( | ||
"{CARRIED} the change this pull makes at `{commit}` is the one reviewed at `{at}`, \ | ||
so that review stands. What is new comes from its base." | ||
); | ||
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); | ||
return Ok(Some(verdict)); | ||
} | ||
earlier.retain(|e| !e.body.starts_with(CARRIED)); | ||
if seen || earlier.len() >= self.opts.rounds { | ||
return Ok(None); | ||
} | ||
let asked = Asked { | ||
| @@ -375,6 +399,29 @@ impl Reviewer { | ||
Ok(Some(answer.verdict)) | ||
} | ||
| ||
/// Whether `pull` changes at `now` what it changed at `before`: the | ||
/// same hunks against its base, wherever they fall, as a merge of the | ||
/// base into the branch leaves them. Not, when `before` is not to be | ||
/// had, as after a force-push that dropped it, or when there is no | ||
/// change at all. | ||
async fn same_change( | ||
&self, | ||
url: &RepoUrl, | ||
pull: &Item, | ||
before: &str, | ||
now: &str, | ||
) -> Result<bool> { | ||
let base = pull.base.as_deref().context("a pull has a base")?; | ||
let head = pull.head.as_ref().context("a pull has a head")?; | ||
let git = knotless::mirror::synced(&self.ctx, url).await?; | ||
let fetched = forge::head_of(&self.ctx, &git, head).await?; | ||
fetched.map_err(anyhow::Error::msg)?; | ||
let base = git.rev_parse(&format!("refs/heads/{base}")).await?; | ||
// One it cannot read, as when it is gone, is no match. | ||
let before = change_id(&git, &base, before).await.ok().flatten(); | ||
Ok(before.is_some() && before == change_id(&git, &base, now).await?) | ||
} | ||
| ||
/// What `.knotless/review.md` says on `url`'s branch `base`, if it is | ||
/// there: the base's, so a pull cannot change how it is reviewed. | ||
async fn instructions(&self, url: &RepoUrl, base: Option<&str>) -> Option<String> { | ||
| @@ -431,6 +478,18 @@ fn earlier(comments: &[forge::Entry], me: &str) -> Vec<Earlier> { | ||
reviews.collect() | ||
} | ||
| ||
/// What `head` changes against where it left `base`, as `git patch-id` | ||
/// names it: its hunks with no context and no line numbers, so a base | ||
/// that moved around them leaves it the same. None for no change. | ||
async fn change_id(git: &knotless::git::Git, base: &str, head: &str) -> Result<Option<String>> { | ||
let range = format!("{base}...{head}"); | ||
let args = ["diff", "-U0", "--no-color", "--no-ext-diff", &range, "--"]; | ||
let diff = git.run(&args, None).await?; | ||
// Of no change, patch-id names none. | ||
let id = git.text(&["patch-id", "--stable"], Some(&diff)).await?; | ||
Ok(id.split_whitespace().next().map(String::from)) | ||
} | ||
| ||
/// What `command` answers of `asked`, given it on stdin. | ||
async fn run_command(command: &str, asked: &Asked<'_>) -> Result<Answer> { | ||
let mut child = tokio::process::Command::new("sh") | ||
crates/knotless-agent/tests/review.rs+51−0
| @@ -320,6 +320,57 @@ fn an_agent_reviews_pulls() { | ||
assert_eq!(asked(&bot, "Name it"), 2); | ||
owner.git(&work, &["reset", "-q", "--hard", "HEAD~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. | ||
let carried = pull("carried", "Carried", &asking); | ||
wait_until(|| { | ||
owner | ||
.cli(&["list", &url], b"") | ||
.1 | ||
.contains(carried.rsplit('/').next().unwrap()) | ||
}); | ||
once(&opts); | ||
assert_eq!(asked(&bot, "Carried"), 1); | ||
wait_until(|| ok(&owner, &["show", &url, &carried]).contains("changes")); | ||
owner.commit_file(&work, "g", "the base moves on\n"); | ||
owner.git(&work, &["push", "-q", &url, "main"]); | ||
owner.git(&work, &["fetch", "-q", &url, "carried"]); | ||
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/carried"]); | ||
owner.git(&work, &["reset", "-q", "--hard", "HEAD~1"]); | ||
once(&opts); | ||
assert_eq!(asked(&bot, "Carried"), 1); | ||
let (said, lines) = reviews(&bot, &carried); | ||
assert_eq!(said.len(), 2); | ||
assert_eq!(said[1]["verdict"], "changes"); | ||
assert_eq!(said[1]["commit"], merged.as_str()); | ||
assert!( | ||
said[1]["body"] | ||
.as_str() | ||
.unwrap() | ||
.starts_with("Carried forward:") | ||
); | ||
assert_eq!(lines.len(), 1); | ||
// Read again at the same head, it writes nothing more. | ||
wait_until(|| ok(&owner, &["show", &url, &carried]).contains(&merged[..12])); | ||
once(&opts); | ||
assert_eq!(reviews(&bot, &carried).0.len(), 2); | ||
let fixed = owner.commit_file(&work, "f", "carried, as asked\n"); | ||
owner.git( | ||
&work, | ||
&["push", "-q", "-f", &url, "HEAD:refs/heads/carried"], | ||
); | ||
once(&opts); | ||
assert_eq!(asked(&bot, "Carried"), 2); | ||
let given_carried = given(&bot, "Carried"); | ||
assert_eq!(given_carried["round"], 2); | ||
assert_eq!(given_carried["head"]["commit"], fixed.as_str()); | ||
assert_eq!(given_carried["earlier"].as_array().unwrap().len(), 1); | ||
assert_eq!(given_carried["earlier"][0]["body"], "Name the file."); | ||
owner.git(&work, &["reset", "-q", "--hard", "HEAD~1"]); | ||
| ||
// One pushed to as it is read is read again at its new head. | ||
let moving = pull("moving", "Moving", &asking); | ||
wait_until(|| { | ||
docs/design/collaboration.md+2−1
| @@ -94,7 +94,8 @@ 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. What it reviewed it reads back | ||
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 | ||
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+2−0
| @@ -109,6 +109,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. | ||
| ||
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. | ||
| ||
The agent keeps no state besides its login session. It learns what it has reviewed by reading back its own reviews, so it can restart, or move to another machine, without reviewing anything twice. | ||
docs/reference/cli.md+5−1
| @@ -570,7 +570,11 @@ 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 | ||
commit, up to `--rounds` times. It never reviews its own. | ||
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 | ||
is written at the new head, beginning `Carried forward:`, and counts as no | ||
round. | ||
| ||
The command reads one pull as JSON on standard input: `repo` | ||
(`knotless://<owner>/<name>`), `pull` (its AT-URI), `title`, `body`, | ||