Keep user-visible wording a human-review reason; complete the README summary - #16
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - a refactor, rename, or move that keeps behaviour; | ||
| - new or extended tests; | ||
| - comments, docs, or wording changes; | ||
| - comments, docs, or wording a user does not see; |
There was a problem hiding this comment.
LOW — Narrowing this non-reason to "wording a user does not see" removes user-visible wording from the non-reasons, but it does not make it a reason. The reasons list above is closed — line 111 reads "Answer true only when a line in the diff does one of these" — and none of the seven clauses covers a relabelled destructive action or a reworded error message: clause 1 enumerates API/type/CLI flag/wire format/config key, and clause 4 is a default value or a removed feature, not a string. So a webui PR that renames "Delete" to "Remove" still falls through to line 145's "When unsure, answer false", which is the case the PR description says this change exists to catch.
The intent needs a positive clause in the list above — e.g. "changes wording a user sees where the new wording could mislead: a destructive action's label, a warning, an error message" — rather than only a subtraction from the list below.
Graded LOW rather than MEDIUM: under the amended rubric false is the specified answer, so nothing here contradicts an executable claim; the gap is between the file and the stated goal.
| default or removes a feature, deletes or loosens a test, migrates persisted | ||
| data, or does what the repo's own review guidelines say a person decides. The | ||
| PR description, deferred work, size, refactors, new tests and wording a user | ||
| does not see are listed there as non-reasons; the rubric is the full list. `human-review-paths` is the floor under that judgement: it forces |
There was a problem hiding this comment.
LOW — This line is ~148 characters; the rest of the paragraph, and the file, wrap at ~76. The rewrite added text mid-paragraph without rewrapping, so the non-reasons sentence and the human-review-paths sentence run together on one line.
| does not see are listed there as non-reasons; the rubric is the full list. `human-review-paths` is the floor under that judgement: it forces | |
| does not see are listed there as non-reasons; the rubric is the full list. | |
| `human-review-paths` is the floor under that judgement: it forces |
|
Two findings: two LOW. A human must look at this one. Both are docs-only changes, and both are accurate as far as they go: the README summary now carries the "what can merge" clause, the runtime-only qualifier on dependencies and the repo-guidelines criterion, and it says the rubric is the full list, which is what the previous phrasing overclaimed. LOW — LOW — A human must look: Unrelated to the diff: |
There was a problem hiding this comment.
Needs a human review. 2 finding(s): 2 LOW.
Nothing blocks, but this change is one a person should decide on:
- review/rubric.md:138 changes when the shared gate withholds approval; consumers pin @master, so it changes what can merge in four downstream repos on merge
1 participant
Summary
Follow-up to #15, which merged before this landed. Two LOW findings from its own review:
review/rubric.md: "comments, docs, or wording changes" as a blanket non-reason would auto-approve product copy intruenas/webui(a relabelled destructive action, a reworded error). Now "wording a user does not see".README.md: the summary of the section claimed to be the full list but omitted the "what can merge" clause, the runtime-only qualifier on dependencies, and the repo-guidelines criterion. It now carries all three and says the rubric is the full list.No Jira ticket on hand for the title; add one so
PR ticketpasses.