159 lines
7.3 KiB
Markdown
159 lines
7.3 KiB
Markdown
# PR#33: a reused line comment gets the text of the new review
|
|
|
|
> **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
|
|
|
|
Since PR#20, a finding near an open line comment of an earlier bot review reuses that comment instead of repeating it.
|
|
The earlier comment keeps its old text, though, and the result comment shows only the title of the new finding.
|
|
The model words a finding differently in each round and often sharpens it after a fix;
|
|
the new wording is lost, and the author answers a text that the latest review no longer holds.
|
|
|
|
The reviews of this PR showed two more defects of 0.20.1:
|
|
it reused a reply of claude-code in the thread of a bot line comment and counted it as an open hint;
|
|
the review request of the YELLOW rounds blocked the merge that the later GREEN review scheduled;
|
|
and a finding at a line beyond the end of its file made Gitea refuse the review with HTTP 500, which ended the run with ERROR.
|
|
|
|
## Non-Goals
|
|
|
|
- No new line comment and no reply per round: the thread stays one comment, so the count of open hints stays right.
|
|
- No edit of a line comment that a human resolved: resolved comments are not reused.
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: a reused line comment gets the text of the new review
|
|
|
|
#### Scenario#33.01: a reused line comment gets the new text
|
|
|
|
So that the author reads and answers what the latest review found.
|
|
|
|
- **Given** an open bot line comment at line 42 with the text "Off by one, old words"
|
|
- **When** the review of a new head finds the same defect at line 40 with the text "Off by one, new words"
|
|
- **Then** the line comment holds the text "Off by one, new words"
|
|
- **and** it names the new head
|
|
- **and** the result comment links it
|
|
|
|
##### Verified by
|
|
|
|
- [test_line_comments: test_a_reused_line_comment_gets_the_new_text](../../tests/test_line_comments.py)
|
|
|
|
#### Scenario#33.02: a reused line comment with the same text stays untouched
|
|
|
|
So that Gitea does not mark a comment as edited without a change.
|
|
|
|
- **Given** an open line comment of an earlier bot review
|
|
- **When** werkrevisor reviews a new head with the same finding in the same words
|
|
- **Then** the bot does not edit the line comment
|
|
|
|
##### Verified by
|
|
|
|
- [test_line_comments: test_a_reused_line_comment_with_the_same_text_stays_untouched](../../tests/test_line_comments.py)
|
|
|
|
#### Scenario#33.03: the new text goes into the result comment if the line comment cannot take it
|
|
|
|
So that the new text is never lost.
|
|
|
|
- **Given** an open line comment of an earlier bot review
|
|
- **When** `<cause>`
|
|
- **Then** the result comment holds the full new text below the link to the earlier line comment
|
|
|
|
| cause |
|
|
|---|
|
|
| Gitea refuses the edit |
|
|
| another finding of the same review already updated this line comment |
|
|
|
|
##### Verified by
|
|
|
|
- [test_line_comments: test_the_new_text_goes_into_the_result_comment_if_the_line_comment_cannot_take_it](../../tests/test_line_comments.py)
|
|
|
|
### Feature: only comments of the bot count as its line comments
|
|
|
|
#### Scenario#33.04: a reply in the thread of a bot line comment is not reused
|
|
|
|
So that the bot never edits the text of a human.
|
|
|
|
- **Given** a resolved bot line comment at line 42 with an open reply of a human in its thread
|
|
- **When** the review of a new head finds a defect at line 40
|
|
- **Then** the bot posts a new line comment and edits no comment
|
|
|
|
##### Verified by
|
|
|
|
- [test_line_comments: test_a_reply_in_the_thread_of_a_bot_review_is_not_reused](../../tests/test_line_comments.py)
|
|
|
|
#### Scenario#33.05: an open reply in the thread of a hint counts as no hint
|
|
|
|
So that an answered hint releases the merge, and a reply never counts twice.
|
|
|
|
- **Given** `[gate] automerge = true`
|
|
- **and** a `<hint>` bot line comment with an open reply of a human in its thread
|
|
- **When** the review of the same head runs again with the verdict GREEN
|
|
- **Then** the merge `<merge>`
|
|
|
|
| hint | merge |
|
|
|---|---|
|
|
| resolved | is scheduled |
|
|
| open | waits for 1 open hint |
|
|
|
|
##### Verified by
|
|
|
|
- [test_hints_hold_merge: test_an_open_reply_to_a_resolved_hint_does_not_hold_the_merge](../../tests/test_hints_hold_merge.py)
|
|
- [test_hints_hold_merge: test_an_open_reply_to_an_open_hint_is_no_second_hint](../../tests/test_hints_hold_merge.py)
|
|
|
|
### Feature: a GREEN review releases its review request
|
|
|
|
#### Scenario#33.06: a scheduled merge withdraws the review request of an earlier round
|
|
|
|
So that a GREEN pull request merges without waiting for a human.
|
|
|
|
- **Given** an open review request to the human reviewer alice from an earlier YELLOW review
|
|
- **When** the GREEN review of a new head schedules the merge
|
|
- **Then** alice is no longer requested as reviewer
|
|
|
|
##### Verified by
|
|
|
|
- [test_hints_hold_merge: test_a_scheduled_merge_withdraws_the_review_request_of_an_earlier_round](../../tests/test_hints_hold_merge.py)
|
|
- [test_hints_hold_merge: test_a_request_listed_only_as_review_is_withdrawn_too](../../tests/test_hints_hold_merge.py)
|
|
- [test_hints_hold_merge: test_a_merge_not_scheduled_keeps_the_review_request](../../tests/test_hints_hold_merge.py)
|
|
- [test_hints_hold_merge: test_a_held_merge_keeps_the_review_request](../../tests/test_hints_hold_merge.py)
|
|
- [test_hints_hold_merge: test_a_failed_withdrawal_is_named_and_keeps_the_review_alive](../../tests/test_hints_hold_merge.py)
|
|
|
|
### Feature: a review with a line beyond the file still completes
|
|
|
|
#### Scenario#33.07: a line comment refused with HTTP 500 goes into the result comment
|
|
|
|
So that a wrong line number of the model does not end the review with ERROR;
|
|
Gitea 1.27 answers a line beyond the end of the file with HTTP 500.
|
|
|
|
- **Given** a finding of the model at a line of a changed file
|
|
- **When** Gitea refuses the line comment with HTTP 500
|
|
- **Then** the result comment holds the finding, as after a refusal with HTTP 422
|
|
|
|
##### Verified by
|
|
|
|
- [test_line_comments: test_finding_without_a_usable_line_stays_in_the_result](../../tests/test_line_comments.py)
|
|
- [test_line_comments: test_a_green_review_refused_with_http_500_approves_without_line_comments](../../tests/test_line_comments.py)
|
|
|
|
## The Solution
|
|
|
|
- werkrevisor reads the id and the text of the open line comments of earlier bot reviews.
|
|
- A finding that reuses such a comment replaces its text by `PATCH /issues/comments/{id}`,
|
|
with a line that names the head of the updating review;
|
|
without a change of the text, ignoring that line, line endings and trailing blanks, it does not edit,
|
|
since Gitea may return a body normalized otherwise than posted.
|
|
- A failed edit, or a second finding at the same comment, puts the full text into the result comment.
|
|
- Only comments written by the bot count as its line comments:
|
|
Gitea lists a reply among the comments of the review that started the thread.
|
|
- A GREEN review that schedules the merge withdraws the open review requests of the configured human reviewers
|
|
(`DELETE /pulls/{n}/requested_reviewers`), listed by the PR or as `REQUEST_REVIEW` review;
|
|
a held or unscheduled merge keeps them,
|
|
and a failed withdrawal is named in the gate note.
|
|
- An HTTP 500 on the review with line comments counts as refusal, like 400 and 422:
|
|
the line comments go into the result comment, and an approval is submitted again without them.
|
|
- werkrevisor 0.20.2.
|
|
|
|
## Prerequisite PRs
|
|
|
|
- PR#32: open hints hold the automerge of a GREEN pull request.
|