154 lines
7.1 KiB
Markdown
154 lines
7.1 KiB
Markdown
# PR#116: Does the PR Document Require Each Change?
|
|
|
|
Version: minor
|
|
|
|
> **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.
|
|
|
|
## Problem
|
|
|
|
A PR can hold a change that nothing in its PR document requires, e.g. a by-catch that is not declared,
|
|
or a change smuggled into an unrelated PR.
|
|
The comparison of PR#47 asks only whether the change summary contradicts a section of the PR document;
|
|
a change that no section mentions does not contradict anything.
|
|
PR#47 also asked eight questions whether the PR document omits a change, e.g. of the API or the database.
|
|
They did not separate: unchanged documents reached up to 0.89.
|
|
Its follow-up idea was one question per summarized change instead of one per document.
|
|
|
|
## Non-Goals
|
|
|
|
- No change of the four contradiction questions of PR#47.
|
|
- The PR document must not lower the judgments about security or weakened checks.
|
|
- A PR document that claims every change in vague words can still hide one; Jev judges the text as written.
|
|
|
|
## Specification
|
|
|
|
### Feature: a change that the PR document does not require
|
|
|
|
#### Scenario#116.01: a change that the PR document does not require gets a hint
|
|
|
|
… so that a by-catch gets declared, or a change that does not belong to the PR gets reverted.
|
|
|
|
- **Given** the change summary holds a change that Jev rates as unrelated to the PR document from `unrelated_min` on
|
|
- **When** werkrevisor reviews the PR with `[doc_check]`
|
|
- **Then** a hint at the title of the PR document names the change and its files
|
|
|
|
##### Verified by
|
|
|
|
- [test_doccheck: test_a_change_that_the_pr_document_does_not_require_gets_a_hint](../../tests/test_doccheck.py)
|
|
|
|
#### Scenario#116.02: a change below `unrelated_min` gives no hint
|
|
|
|
… so that a change that the PR document requires does not hold the merge.
|
|
|
|
- **Given** Jev rates every change of the summary below `unrelated_min`
|
|
- **When** werkrevisor reviews the PR with `[doc_check]`
|
|
- **Then** the result names the number of changes that agree with the PR document
|
|
- **and** no hint names a change
|
|
|
|
##### Verified by
|
|
|
|
- [test_doccheck: test_a_required_change_gives_no_hint](../../tests/test_doccheck.py)
|
|
|
|
#### Scenario#116.03: Jev judges each change with the conventions of the repository
|
|
|
|
… so that a version raise or a plan update needs no mention in the PR document.
|
|
|
|
- **Given** the change summary holds two changes
|
|
- **When** werkrevisor reviews the PR with `[doc_check]`
|
|
- **Then** Jev gets one state per change, with the PR document, that change and the conventions
|
|
|
|
##### Verified by
|
|
|
|
- [test_doccheck: test_jev_judges_each_change_with_the_pr_document_and_the_conventions](../../tests/test_doccheck.py)
|
|
|
|
#### Scenario#116.04: a resolved hint is not repeated while the PR document and the files stay unchanged
|
|
|
|
… so that the author can answer a hint once.
|
|
|
|
- **Given** the author resolved the hint about a change
|
|
- **When** werkrevisor reviews a new commit with the same PR documents and a change of the same files
|
|
- **Then** werkrevisor does not repeat the hint
|
|
|
|
##### Verified by
|
|
|
|
- [test_doccheck: test_a_resolved_change_hint_is_not_repeated](../../tests/test_doccheck.py)
|
|
|
|
#### Scenario#116.05: the usage record logs the value per change
|
|
|
|
… so that `unrelated_min` can be calibrated on real PRs.
|
|
|
|
- **When** werkrevisor writes the usage record
|
|
- **Then** the record holds the files, the kind and the value of each judged change
|
|
|
|
##### Verified by
|
|
|
|
- [test_doccheck: test_the_usage_record_logs_the_value_per_change](../../tests/test_doccheck.py)
|
|
|
|
## Solution
|
|
|
|
- After the comparison per section, Jev judges each change of the summary once:
|
|
the state holds the changed PR documents, the change and the conventions of the repository.
|
|
- A new key `[doc_check] unrelated_min`, default 0.7, sets the value from which a change gets a hint.
|
|
- The hint is a finding at the first line of the PR document, like the other hints of the comparison;
|
|
it holds a fingerprint of the files of the change and of the PR documents.
|
|
- The usage record holds the values as `doc_coverage_judgments`; `doc_check_calls` counts the calls too.
|
|
- The diagrams stay unchanged: the step "PR document vs diff" covers the new question, and no decision changes.
|
|
- werkrevisor 0.44.0.
|
|
|
|
## Measurement
|
|
|
|
On 2026-10-17 a script tested one Jev call per summarized change on 36 merged PRs:
|
|
werkrevisor #99 to #114, werkator #122 to #131, werkregie #62 to #64, #66 and #68 to #73.
|
|
MiniMax M3 wrote the summaries without the PR documents, as in the review, for 0.001 to 0.008 USD per PR;
|
|
they held 181 changes.
|
|
Each PR also got 3 planted changes from the summaries of other PRs:
|
|
2 of the same repository and 1 of another one, without docs, build, tests, version and plan changes.
|
|
The state of Jev held the PR document and one change, about 1500 tokens.
|
|
|
|
Three questions were measured, as a value for "the PR document does not require the change":
|
|
|
|
| question | AUC, same repository | AUC, other repository |
|
|
|---|---|---|
|
|
| does the PR document describe, require or justify the change (inverted) | 0.990 | 1.000 |
|
|
| is the change unrelated, so that a reviewer would ask why it belongs here | 0.995 | 1.000 |
|
|
| would the change surprise a reviewer who knows only the PR document | 0.992 | 1.000 |
|
|
|
|
Without further context, almost all real changes above 0.5 were routine changes that the workflow requires:
|
|
version raises, Werkbaum updates and release notes.
|
|
So the state also got the conventions of the repository:
|
|
the line `Version: …` requires a version raise,
|
|
and version, plan, release notes, tests, documentation and diagrams belong to a PR without a mention.
|
|
With them, the question "unrelated" gave:
|
|
|
|
| threshold | real changes | planted, same repository | planted, other repository |
|
|
|---|---|---|---|
|
|
| 0.5 | 5 of 181, in 3 PRs | 70 of 72 | 36 of 36 |
|
|
| 0.6 | 4 of 181, in 3 PRs | 69 of 72 | 36 of 36 |
|
|
| 0.7 | 3 of 181, in 2 PRs | 66 of 72 | 36 of 36 |
|
|
| 0.8 | 0 of 181 | 62 of 72 | 36 of 36 |
|
|
|
|
The 3 real changes from 0.7 on are version raises in werkrevisor #100 and #103;
|
|
both PR documents predate the line `Version: …` and do not mention a version.
|
|
The fourth one from 0.6 on is a CSS refactoring in werkregie #73 that its PR document does not mention.
|
|
The planted changes below 0.7 touch the same area as the PR, e.g. the package move of #113 planted into #111;
|
|
a reviewer could also take them as part of the PR.
|
|
The questions of PR#47 differ in one point: they asked about the whole document, these ask about one change.
|
|
|
|
## Release Notes
|
|
|
|
- With `[doc_check]`, a change that the PR document does not require gets a hint at the title of the PR document.
|
|
|
|
## Open Questions
|
|
|
|
- [x] Does the question per summarized change separate?
|
|
Yes: with the conventions, 0 of 181 real changes and 98 of 108 planted ones from 0.8 on; see "Measurement".
|
|
- [x] Hint or ORANGE?
|
|
A hint from 0.7 on, as the contradictions of PR#47 started; it holds the merge until the author resolves it.
|
|
Real smuggled changes are less obvious than planted ones; ORANGE needs a calibration on real PRs first.
|
|
|
|
## Follow-up PRs
|
|
|
|
- Calibrate `unrelated_min` on the usage records of real PRs, and decide about ORANGE.
|