docs: clarify PR fixup review authorization (#3888)
This commit is contained in:
@@ -76,7 +76,12 @@ Ordinary PR-head checks do not validate a merge-group commit.
|
|||||||
Directly supplied review findings need current-source validation even without
|
Directly supplied review findings need current-source validation even without
|
||||||
a GitHub thread. Apply valid fixes within the user's authorization.
|
a GitHub thread. Apply valid fixes within the user's authorization.
|
||||||
Do not invent comment IDs or resolutions for findings without a thread.
|
Do not invent comment IDs or resolutions for findings without a thread.
|
||||||
External replies and thread resolution still require user authorization.
|
An explicit request to run PR fixup authorizes concise GitHub replies and
|
||||||
|
resolution for every unresolved review thread listed by
|
||||||
|
`scripts/pr-resolve list <PR>` once that thread's disposition is complete. This
|
||||||
|
defines the fixup scope, so do not ask a separate approval question for these
|
||||||
|
writes. A request to address selected comments authorizes writes only for those
|
||||||
|
threads; a review-only request authorizes no GitHub writes.
|
||||||
|
|
||||||
If `reviewDecision=REVIEW_REQUIRED` and `mergeStateStatus=BLOCKED`, report the
|
If `reviewDecision=REVIEW_REQUIRED` and `mergeStateStatus=BLOCKED`, report the
|
||||||
human approval gate. Green checks do not authorize self-approval or merging.
|
human approval gate. Green checks do not authorize self-approval or merging.
|
||||||
@@ -170,21 +175,25 @@ and authoritative spec. Do not silently encode the compromise.
|
|||||||
explain where it is addressed, and resolve the thread when writes are
|
explain where it is addressed, and resolve the thread when writes are
|
||||||
authorized.
|
authorized.
|
||||||
- `informational` or `optional`: make no code change, acknowledge the information
|
- `informational` or `optional`: make no code change, acknowledge the information
|
||||||
or suggestion in a reply, and resolve it when complete cleanup is requested.
|
or suggestion in a reply, and resolve it when included in authorized fixup or
|
||||||
|
cleanup scope.
|
||||||
- `invalid`: do not make an invalid code change or silently ignore the finding;
|
- `invalid`: do not make an invalid code change or silently ignore the finding;
|
||||||
reply with concrete reasoning grounded in the code, spec, or architecture,
|
reply with concrete reasoning grounded in the code, spec, or architecture,
|
||||||
then resolve it only after that pushback is posted and writes are authorized.
|
then resolve it only after that pushback is posted and writes are authorized.
|
||||||
|
|
||||||
A classification is not a disposition. A thread is not complete merely because
|
A classification is not a disposition. A thread is not complete merely because
|
||||||
it was called optional, informational, already addressed, or invalid. GitHub
|
it was called optional, informational, already addressed, or invalid. GitHub
|
||||||
replies and thread resolution are external writes. A request such as "complete
|
replies and thread resolution are external writes. An explicit PR-fixup request
|
||||||
cleanup", "clean up all review threads", or "leave no threads unresolved"
|
authorizes a concise reply and resolution for every unresolved review thread
|
||||||
explicitly authorizes a concise reply and resolution for every unresolved
|
listed by `scripts/pr-resolve list <PR>`, including informational, optional,
|
||||||
thread, including informational, optional, and invalid findings. A request to
|
and invalid findings. This defines the write scope; do not ask separately
|
||||||
address selected comments authorizes replies and resolution only for those
|
before these writes or extend them to comments outside that list. Requests such
|
||||||
selected threads. A review-only request authorizes no writes. When writes are
|
as "complete cleanup", "clean up all review threads", or "leave no threads
|
||||||
not authorized, record every disposition, report the still-unresolved thread,
|
unresolved" authorize this for every unresolved thread. A request to address
|
||||||
and do not declare the PR clean.
|
selected comments authorizes replies and resolution only for those threads. A
|
||||||
|
review-only request authorizes no writes. When writes are not authorized,
|
||||||
|
record every disposition, report the still-unresolved thread, and do not
|
||||||
|
declare the PR clean.
|
||||||
|
|
||||||
For code changes, push the fix and pass targeted verification before replying
|
For code changes, push the fix and pass targeted verification before replying
|
||||||
and resolving. For non-code dispositions, verify the current head first, then
|
and resolving. For non-code dispositions, verify the current head first, then
|
||||||
@@ -365,6 +374,6 @@ For "monitor until merge", poll queue/PR state with bounded cadence; require
|
|||||||
- Do not push, post comments, or resolve threads when the user asked for review
|
- Do not push, post comments, or resolve threads when the user asked for review
|
||||||
only.
|
only.
|
||||||
- Do not silently dismiss an invalid finding; every invalid thread needs a
|
- Do not silently dismiss an invalid finding; every invalid thread needs a
|
||||||
concrete pushback disposition, and an authorized complete cleanup must post
|
concrete pushback disposition, and an authorized fixup or complete cleanup
|
||||||
that explanation before resolving it.
|
must post that explanation before resolving it.
|
||||||
- Do not proceed with an unverified PR when mandatory verification is blocked.
|
- Do not proceed with an unverified PR when mandatory verification is blocked.
|
||||||
|
|||||||
@@ -45,15 +45,17 @@ informational or optional suggestion; or give concrete code/spec/architecture
|
|||||||
reasoning for an invalid finding. Do not treat a label, internal note, or lack
|
reasoning for an invalid finding. Do not treat a label, internal note, or lack
|
||||||
of code change as a completed disposition.
|
of code change as a completed disposition.
|
||||||
|
|
||||||
When the user requests complete cleanup, including wording such as "clean up
|
An explicit request to run PR fixup authorizes a concise reply and resolution
|
||||||
all review threads" or "leave no threads unresolved", reply to and resolve
|
for every unresolved review thread returned by `scripts/pr-resolve list <PR>`,
|
||||||
every unresolved thread after its disposition. This includes informational and
|
after that thread's disposition is complete. This includes informational and
|
||||||
optional threads, which need an acknowledgement, and invalid threads, which
|
optional threads, which need an acknowledgement, and invalid threads, which
|
||||||
need the concrete pushback reply before resolution. A request limited to
|
need the concrete pushback reply before resolution. The listed review threads
|
||||||
selected actionable comments does not authorize writes to other threads. If
|
define the fixup's write scope; do not extend writes to comments outside that
|
||||||
thread writes are not authorized, report each disposition and keep the thread
|
list. A request limited to selected actionable comments authorizes writes only
|
||||||
unresolved; never report the review state as clean. An invalid finding must
|
to those threads. A review-only request authorizes no writes. If thread writes
|
||||||
never be silently ignored.
|
are not authorized, report each disposition and keep the thread unresolved;
|
||||||
|
never report the review state as clean. An invalid finding must never be
|
||||||
|
silently ignored.
|
||||||
|
|
||||||
If `gh`, `scripts/pr-state`, or `scripts/pr-resolve` fails with an
|
If `gh`, `scripts/pr-state`, or `scripts/pr-resolve` fails with an
|
||||||
authentication or transport error (including a broker 401), do not treat empty
|
authentication or transport error (including a broker 401), do not treat empty
|
||||||
|
|||||||
Reference in New Issue
Block a user