135 lines
5.8 KiB
Markdown
135 lines
5.8 KiB
Markdown
# PR#73: An Approval Survives a Rebase
|
|
|
|
> **WARNING:** This document describes only the change applied in this PR.
|
|
> It may already be outdated once the next PR is merged.
|
|
> Historic PR documentation is not maintained along with new PRs; treat it as a snapshot.
|
|
|
|
## The Problem
|
|
|
|
Gitea dismisses an approval when new commits arrive (`dismiss_stale_approvals`).
|
|
A rebase onto `main`, or a merge of `main` into the branch, thus costs a new human approval,
|
|
although the change of the PR stays the same.
|
|
werkator PR#137 needed three approvals of the same change in this way.
|
|
|
|
## Non-Goals
|
|
|
|
- No carry-over of an approval that a human dismissed by hand or revoked with "request changes".
|
|
- No check that the merged `main` still fits the change semantically; the CI (`werkator`) stays required for that.
|
|
- No calibration of the Jev thresholds for corrections; this PR uses those of `[path_judge]`.
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: an approval survives a rebase
|
|
|
|
#### Scenario#73.01: a rebase keeps the approval
|
|
|
|
So that a rebase onto `main` costs no new human approval.
|
|
|
|
- **Given** a human approved the head A of a PR
|
|
- **When** the PR is rebased onto a newer `main` without a conflict
|
|
- **Then** werkrevisor approves the new head without Jev and without a model call
|
|
- **And** its reason names the approval of A
|
|
|
|
##### Verified by
|
|
|
|
- [test_carry_over.CarryOverTest.test_a_rebase_keeps_the_approval](../../tests/test_carry_over.py)
|
|
|
|
#### Scenario#73.02: a small correction after a merge keeps the approval
|
|
|
|
So that a resolved merge conflict needs no human when Jev rates it as harmless.
|
|
|
|
- **Given** a human approved the head A of a PR
|
|
- **And** the new head has a changed block that A did not have
|
|
- **And** Jev rates that block below the thresholds of `[path_judge]`
|
|
- **When** werkrevisor reviews the new head
|
|
- **Then** it approves without a model call
|
|
|
|
##### Verified by
|
|
|
|
- [test_carry_over.CarryOverTest.test_a_harmless_correction_keeps_the_approval](../../tests/test_carry_over.py)
|
|
|
|
#### Scenario#73.03: a risky correction gets a review of the correction only
|
|
|
|
So that approved changes do not block the PR again.
|
|
|
|
- **Given** a human approved the head A of a PR
|
|
- **And** Jev rates a new block as risky
|
|
- **When** werkrevisor reviews the new head
|
|
- **Then** the model gets only the files with new blocks
|
|
- **And** a finding on the lines of an approved block is a hint
|
|
|
|
##### Verified by
|
|
|
|
- [test_carry_over.CarryOverTest.test_a_risky_correction_gets_a_review](../../tests/test_carry_over.py)
|
|
- [test_carry_over.CarryOverTest.test_a_finding_on_approved_lines_is_a_hint](../../tests/test_carry_over.py)
|
|
- [test_carry_over.ChangeKindTest.test_the_kind_comes_from_the_files_outside_the_approval](../../tests/test_carry_over.py)
|
|
|
|
#### Scenario#73.04: an approval dismissed by hand is not carried over
|
|
|
|
So that a human can take back an approval.
|
|
|
|
- **Given** a human dismissed the approval by hand
|
|
- **When** werkrevisor reviews the new head
|
|
- **Then** the model reviews every changed file
|
|
|
|
##### Verified by
|
|
|
|
- [test_carry_over.CarryOverTest.test_an_approval_dismissed_by_hand_is_not_carried_over](../../tests/test_carry_over.py)
|
|
|
|
#### Scenario#73.05: a dropped approved change is not carried over
|
|
|
|
So that a check that the PR added cannot disappear unnoticed in a rebase.
|
|
|
|
- **Given** a human approved the head A of a PR
|
|
- **When** a rebase drops a changed block of A
|
|
- **Then** the next review gives the model every changed file
|
|
- **And** a hint names the number of missing blocks
|
|
|
|
##### Verified by
|
|
|
|
- [test_carry_over.CarryOverTest.test_a_dropped_block_ends_the_carry_over](../../tests/test_carry_over.py)
|
|
|
|
#### Scenario#73.06: blocks approved by werkrevisor do not block again
|
|
|
|
So that a rebase after a GREEN review brings no new blocking findings on unchanged lines.
|
|
|
|
- **Given** werkrevisor approved the head A of a PR as GREEN
|
|
- **When** werkrevisor reviews the rebased head
|
|
- **Then** a finding on a block of A is a hint
|
|
|
|
##### Verified by
|
|
|
|
- [test_carry_over.CarryOverTest.test_blocks_of_a_green_review_do_not_block_again](../../tests/test_carry_over.py)
|
|
|
|
## The Solution
|
|
|
|
- Every result comment gets a hidden marker with a fingerprint per changed block:
|
|
the path, the removed and the added lines, without line numbers and without context.
|
|
- A review looks for the newest approval of a head other than the current one:
|
|
a human approval that Gitea dismissed automatically, or a GREEN approval of werkrevisor itself.
|
|
The fingerprints come from the result comment of werkrevisor for that head.
|
|
- After a human approval, files with approved blocks only are left out of Jev and the model, like mechanical files.
|
|
Without new blocks, werkrevisor approves; with new blocks, Jev judges their files first.
|
|
- A finding of the model on approved lines only is a hint.
|
|
- The change kind, which the model checks, comes from the files outside the approval.
|
|
- `[gate] carry_over_approvals = false` switches the carry-over of human approvals off.
|
|
- The README explains it under "Approval after a rebase"; the decision diagram shows both shortcuts.
|
|
- Version 0.32.0.
|
|
|
|
## Open Questions
|
|
|
|
- [x] Can werkrevisor compare the approved head with the new one through the Gitea API?
|
|
Not directly: the compare route with `.diff` answers 404 for a private repository, even with a token.
|
|
So each review keeps the fingerprints of its own head in its result comment.
|
|
- [x] How does werkrevisor tell an approval dismissed by a push from one dismissed by hand?
|
|
Gitea's timeline has a `dismiss_review` event; after a push its text is
|
|
"New commits pushed, approval review dismissed automatically", by hand it is the text of the human.
|
|
|
|
## Additional Changes
|
|
|
|
- The Werkbaum gets the plan `#werkregie.pwa`: the Werkregie frontend becomes a PWA; this PR does not implement it.
|
|
|
|
## Follow-up PRs
|
|
|
|
- Calibrate the Jev thresholds for corrections on real rebases, e.g. werkator PR#137, see `#gate.carry.calibrate`.
|