From 016b265a728158b0bf8d424b652279c65e389f63 Mon Sep 17 00:00:00 2001 From: William Grzybowski Date: Sat, 12 Sep 2026 12:32:42 -0300 Subject: [PATCH] Narrow "does this need a human" to judgement calls in the diff It was firing on nearly every PR: "touches CI", "user-facing behaviour" and "a choice the PR description presents as a decision" each cover most changes, and the last one flagged deferred work mentioned only in a description. The section now defaults to no, requires each reason to anchor to a line in the diff, narrows every criterion to removal or change rather than addition, and lists what does not count. --- README.md | 16 +++++++------ review/rubric.md | 61 +++++++++++++++++++++++++++++++----------------- 2 files changed, 49 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index ed4877d..8fe8e9a 100644 --- a/README.md +++ b/README.md @@ -290,13 +290,15 @@ cannot disagree with the check: | No or unparseable output | nothing | fails | "A human must look" is the reviewer's own answer to the *Does this need a -human?* section of `review/rubric.md` — public API or exported type changes, a -new or major-bumped dependency, CI, release or auth changes, user-facing -behaviour, wording or default changes, removed tests, data migrations, anything -the PR presents as a decision — plus `human-review-paths`, which forces it for -any PR touching a matching file regardless of what the reviewer said. That -input is the floor: the model's call on "is this a product decision" is the -fuzziest judgement in the pipeline, and a path list does not depend on it. +human?* section of `review/rubric.md`, which defaults to no and says yes only +for a line in the diff that removes or changes something external callers +depend on, adds or major-bumps a dependency, changes a workflow's permissions, +secrets or what it publishes, changes a visible default or removes a feature, +deletes or loosens a test, or migrates persisted data. The PR description, +deferred work, size, refactors, new tests and wording are listed there as +non-reasons. `human-review-paths` is the floor under that judgement: it forces +the answer for any PR touching a matching file regardless of what the reviewer +said, and is where a repo puts the paths it always wants eyes on. Each run adds a review; GitHub reviews are appended, not edited, so a PR with ten pushes carries ten of them, and the newest is the one that describes the diff --git a/review/rubric.md b/review/rubric.md index 397afd4..4710ebe 100644 --- a/review/rubric.md +++ b/review/rubric.md @@ -105,27 +105,46 @@ question: must a person look at this change before it merges, even when nothing is wrong. Report it in `human_review`, and say it in the summary's opening line after the count. -Answer `required: true`, with one reason per line naming the file or change, -when the change does any of these: - -- alters a public API, exported type, CLI, protocol, or file format that - callers depend on; -- adds a dependency, or takes one across a major version; -- touches CI, release, auth, permissions, or secrets handling; -- changes user-facing behaviour, wording, defaults, or whether a feature is - available; -- removes or weakens a test; -- migrates data or changes a schema; -- makes a choice the PR description itself presents as a decision, or that the - repository's own guidelines say a person decides. - -Otherwise answer `required: false` with an empty `reasons`. Size is not a -reason: a large mechanical change covered by its tests does not need a person -because it is large. Nor is a LOW finding — that is already reported as one. - -The workflow submits a PR review from this answer. `false` on a clean change -is what lets the workflow approve it, so the answer is a claim the check acts -on, not a hedge. +The default is `required: false`. A clean change merges on this answer, and +the cost of a wrong `true` is real: it is a person's time on every routine +pull request, and the mechanism exists to spend that time only where a +judgement call sits in the diff. Answer `true` only when a line *in the diff* +does one of these: + +- removes, renames, or changes the meaning of something callers outside this + repository depend on: an exported API or type, a CLI flag, a wire or file + format, a config key. Adding one is not this; +- adds a runtime dependency, or moves one across a major version; +- changes what a workflow or release may do: its permissions, the secrets or + tokens it holds, what it publishes, or who or what can merge; +- changes a default value, or removes or disables a feature or flag, that a + user or caller will see; +- deletes a test, or loosens an assertion so it accepts more than before; +- migrates stored data or changes a persisted schema; +- does something the repository's own review guidelines explicitly say a + person decides. + +Each reason names the file and the change on that line. A reason you cannot +anchor to a line in the diff is not a reason. + +These are not reasons, however much they might feel like one: + +- the PR description, title, or commit messages: what they say, defer, or + leave for later. Work not in the diff is not in the review; +- a LOW finding: it is already reported as one; +- size, or the number of files touched; +- a refactor, rename, or move that keeps behaviour; +- new or extended tests; +- comments, docs, or wording changes; +- a CI or workflow change that does not change permissions, secrets, what is + published, or what gates a merge; +- code that adds a case, branch, or handler alongside existing ones without + changing what existed. + +The paths a repository always wants a person on are configured by that +repository (`human-review-paths`), so do not guess at them here. When unsure, +answer `false`: a person can still review a pull request the workflow did not +ask them to, and the findings list is where anything wrong belongs. ## Machine-readable summary