chadtmiller.com/knotless

Switch repository

Loading…

What changes

A head that brings nothing of its own is not reviewed again. Before reading a pull, the reviewer compares what the pull changes against its base at the new head with what it changed at the last head reviewed. Each is the git patch-id --stable of a -U0 diff from the merge base, so the comparison ignores where the hunks fall in each file. If they match, as after a merge of the base into the branch, no command runs. The last verdict is written at the new head in a review beginning Carried forward:, which says which head it was carried from. A carried review is not counted toward --rounds, and the command is not given it among earlier. A real change after it is read as the next round.

Never twice at one head. The reviewer also skips a head that its own thread already has a review at, even when the appview's view of the pull has not heard of that review yet. Without this, a carried review that the appview hasn't heard of yet would be read again as a fresh round.

The deploy reviews up to five rounds (KNOTLESS_REVIEW_ARGS: --all --rounds 5).

The reviews guide, the CLI reference and the collaboration design note say how carrying works.

Why

grain's move off hatk runs as a feature branch with several pulls open into it at once. Each time one lands, the others merge the branch in, and each merge was a new head that spent a round. One pull spent all three rounds on heads like that. The commits that came after them (a cursor fix, and writes stored at each commit's revision) were never read by the reviewer, and were read by hand instead.

Raising the cap alone would pay for more re-reads of merges. Carrying the verdict keeps the cap meaning "reviews of this pull's own change".

How it was checked

an_agent_reviews_pulls gains a case:

• A pull is reviewed once and asked for a change.
• The base moves, and the base is merged into the pull's branch. The reviewer carries changes to the merge without running the command, writes no line comments, and does not count it as a round.
• Read again at the same head, it writes nothing more.
• A real change to the pull is then read as round 2, given only the first review as earlier.

Nothing was built or run locally. CI runs fmt, clippy and the tests.

Shortcomings

• If the merge's conflict resolution touches the pull's own lines, the patch-id changes and the head is reviewed in full. That is the intent, but resolutions that keep the same lines and move context are still treated as unchanged.
• An old head that a force-push dropped from the mirror can't be compared, so it is reviewed in full, as before.
• A carried changes verdict stays changes until the author pushes a real change. Nothing outside the pull's own change is reconsidered.

Review

Start at Reviewer::review and same_change in crates/knotless-agent/src/lib.rs.

🤖 Generated with Claude Code

This makes the reviewer carry its last verdict to a new head when git patch-id --stable of the -U0 diff from the merge base is unchanged. That covers a plain merge of the base into the branch. The carried review begins Carried forward:, is dropped from earlier and the round count, and a head the thread already has a review at is skipped.

I read the control flow in Reviewer::review and found nothing that must change:

• The head is re-checked before the carried comment is written.
• posted is set after it, so the same head is not carried twice.
• A review with no commit, or an old head that is gone, falls through to a full review.
• A real change after a carry reads as round 2 with only the real review in earlier, which the new test checks.
• The docs the repository's AGENTS.md asks for (guide, CLI reference, collaboration design) are updated. The change touches only knotless-agent, so it needs no changeset.

Two things to know, neither of which blocks:

• A -U0 patch-id ignores context. A merge resolution that keeps the pull's added lines but changes how they interact with new base code is carried as unchanged. You list this as a shortcoming, and I think it is acceptable for the cap's purpose.
• same_change uses ? on the mirror sync, head_of and the now diff. A transient failure there makes the sweep note a warning and retry, rather than falling back to a full review. That seems fine.

Nothing was built or run, so I'm relying on CI for fmt, clippy and the tests.