111 lines
4.5 KiB
Markdown
111 lines
4.5 KiB
Markdown
# Review notes protocol
|
|
|
|
Date: 2026-10-02
|
|
Status: approved
|
|
|
|
## Purpose
|
|
|
|
Record code-review results as git notes under `refs/notes/review` so that the
|
|
next reviewer — human or AI — can read the current verdict, see open findings
|
|
with stable IDs, and continue from where the last review stopped. This
|
|
replaces a CI reviewer: reviews happen locally, results travel with the
|
|
commits they judged.
|
|
|
|
## Storage model
|
|
|
|
- One note per reviewed commit, in the `review` notes ref.
|
|
- The tip of a reviewed branch always carries the latest verdict.
|
|
- Re-reviewing after fixes writes a **new** note on the **new** tip commit.
|
|
Old notes stay attached to old commits as history.
|
|
- Each note links to the previously reviewed commit via `previous`, forming a
|
|
walkable chain of review rounds per branch.
|
|
- Merging to main needs no special handling: the `approve` note on the final
|
|
tip is the record.
|
|
|
|
## Note format
|
|
|
|
Line-based header, then a blank line, then free markdown body:
|
|
|
|
branch: <branch name>
|
|
round: <integer, starting at 1>
|
|
verdict: approve | request-changes
|
|
previous: none | <sha of previously reviewed tip>
|
|
findings:
|
|
- id: F<N>
|
|
severity: blocker | major | minor
|
|
file: <path>:<line>
|
|
title: <one line>
|
|
status: open | resolved | wontfix
|
|
## Details
|
|
|
|
<markdown prose>
|
|
|
|
Rules:
|
|
|
|
- Finding IDs are unique within a note and carry forward across rounds: a
|
|
finding resolved since the last note keeps its ID and moves to
|
|
`status: resolved`; an unfixed finding keeps its ID and `status: open`.
|
|
New findings get fresh IDs that do not collide with carried-forward ones.
|
|
- The body is free markdown; the `## Details` heading in the example is
|
|
illustrative, not required.
|
|
- Headers are plain `key: value` lines so the helper script can parse them
|
|
with sed/awk.
|
|
|
|
## Helper script: `scripts/review-note`
|
|
|
|
Bash, git plumbing only. Subcommands:
|
|
|
|
- `review-note show [commit]` — print the note for a commit (default: current
|
|
tip). Exit 1 with a message on stderr if there is no note.
|
|
- `review-note write <file>` — attach the note in `<file>` to the current tip
|
|
commit. Refuses to replace an existing note unless `--force` is given; a
|
|
proper re-review targets a new commit, so overwriting is always deliberate.
|
|
Validates that `branch`, `round`, and `verdict` are present and that
|
|
`verdict` uses the vocabulary above. No other validation.
|
|
- `review-note latest [branch]` — print the note with the highest `round` for
|
|
a branch (default: current branch), scanning the notes ref and matching
|
|
notes on their `branch:` header. Exit 1 if there is no note for the branch.
|
|
(A `previous`-chain walk cannot bootstrap from an unreviewed tip, so the
|
|
scan uses the `branch:` header as the index instead.)
|
|
- `review-note push` — `git push origin refs/notes/review`. Plain `git push`
|
|
ignores notes, so this is the only way results travel.
|
|
|
|
`push` pushes without force; if the remote rejects, report the failure and let
|
|
the user decide.
|
|
|
|
## Lifecycle
|
|
|
|
1. Reviewer reads `review-note latest <branch>` (exit 1 means first review).
|
|
2. Reviewer reviews the branch tip per `REVIEW.md`.
|
|
3. Reviewer writes the note with `review-note write` and runs
|
|
`review-note push`.
|
|
4. If the verdict is `request-changes`, the author pushes fixes as new
|
|
commits. The next review round starts at the new tip with `round`+1,
|
|
`previous` set to the last reviewed tip, and carried-forward findings.
|
|
5. When the verdict is `approve` and the branch merges, the notes remain on
|
|
the branch commits as the review record.
|
|
|
|
## Documentation integration
|
|
|
|
- `REVIEW.md` gains a "Recording reviews" section describing the protocol and
|
|
the four script commands. This is the single source of truth for the
|
|
protocol.
|
|
- `AGENTS.md` "Reviewing" section gains one line pointing at the section, so
|
|
any AI session picks up the flow automatically.
|
|
|
|
## Seeding the experiment
|
|
|
|
The failing review of `home_assistant_controller_cleanup` (commit `ba6e0e8`)
|
|
becomes note round 1, with these findings:
|
|
|
|
- F1 (blocker, src/AFRP.hs:273): hlint's `\_ -> x` → `const x` rewrites break
|
|
the rank-2 `Mealy` type; library does not compile.
|
|
- F2 (blocker, test/AFRPLawsSpec.hs:98): seven law tests reduced to
|
|
tautologies by hlint rewrites; see :103, :212, :221, :228, :235, :256.
|
|
|
|
## Verification
|
|
|
|
The script is verified by round-trip checks in a throwaway temp clone: write,
|
|
show, latest (two rounds, branch filtering), force-overwrite refusal and
|
|
acceptance, and push. No Haskell or cabal changes, so no
|
|
cabal2nix regeneration. |