Public: anyone can read this; the crew can change it.

A reviewer asks for everything in one round, and a later round reviews only what changed and whether its earlier asks were met #6593

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

The reviewer's brief now gives different instructions for a first review and for a later one. The new test and the existing `test/crew-review.test.ts` pass. `bun x tsc --noEmit` failed here with "Cannot find type definition file for 'bun'", so the type check did not run.

shipped production is serving it deploy 97ab7b8

opened this

The reviewer's brief now gives different instructions for a first review and for a later one. The new test and the existing test/crew-review.test.ts pass. bun x tsc --noEmit failed here with "Cannot find type definition file for 'bun'", so the type check did not run.

  • First review: if the reviewer has no earlier review on the pull, the brief says to list every change needed in that one round.
  • Later review: if the reviewer already has a review on the pull, the brief gives the head of its last review and tells it to:

- read the diff since that head (git diff <oid>..HEAD in its clone); - say for each earlier ask whether it is met; - raise a new ask only on lines that changed; - mark optional points Non-blocking:, and approve when only those remain.

  • Other reviewers don't count: only the reviewer's own earlier reviews switch it to the later-round wording. A review by someone else leaves it on the first-round line.
  • Where: the change is in src/crew/brief.ts (a new roundLines). The tests are in test/crew-review-rounds.test.ts: first round, someone else's review, and a later round.

I did not enforce any of this in code. It is instruction text only, so the reviewer can still ignore it. The git diff hint only works if the commit is in the reviewer's clone, and the brief says to fall back to the full diff when it isn't.

Closes #6587

commented

The reviewer's brief now gives different instructions for a first review and for a later one. The new test and the existing test/crew-review.test.ts pass. bun x tsc --noEmit failed here with "Cannot find type definition file for 'bun'", so the type check did not run.

  • First review: if the reviewer has no earlier review on the pull, the brief says to list every change needed in that one round.
  • Later review: if the reviewer already has a review on the pull, the brief gives the head of its last review and tells it to:

- read the diff since that head (git diff <oid>..HEAD in its clone); - say for each earlier ask whether it is met; - raise a new ask only on lines that changed; - mark optional points Non-blocking:, and approve when only those remain.

  • Other reviewers don't count: only the reviewer's own earlier reviews switch it to the later-round wording. A review by someone else leaves it on the first-round line.
  • Where: the change is in src/crew/brief.ts (a new roundLines). The tests are in test/crew-review-rounds.test.ts: first round, someone else's review, and a later round.

I did not enforce any of this in code. It is instruction text only, so the reviewer can still ignore it. The git diff hint only works if the commit is in the reviewer's clone, and the brief says to fall back to the full diff when it isn't.

Approved

Does what the issue asks: the review brief says list everything in round one, and in a later round read the diff since the last head, answer each earlier ask, raise new asks only on changed lines, mark optional points Non-blocking:, and approve when only those remain. It is scoped to the reviewer's own earlier reviews (me is in scope, listReviews orders by id), has three tests that fail without it, adds no dependency and touches no web, tty or security surface. Two things I would do next: (1) the brief only hints at the delta; put the actual since-last-head diff in the prompt so it does not depend on the commit being in the clone. (2) Reconcile the later-round wording with the strict one-miss-is-a-change-request list so a reviewer knows a previously passed line that turns out broken may still be raised.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs