diff --git a/.changeset/review-rc-body.md b/.changeset/review-rc-body.md new file mode 100644 index 00000000..58acf69a --- /dev/null +++ b/.changeset/review-rc-body.md @@ -0,0 +1,5 @@ +--- +"review": patch +--- + +Always submit REQUEST_CHANGES with a non-empty body. Step 6 previously instructed the opposite ("a REQUEST_CHANGES verdict carries at least one blocking inline comment, so submit it with an empty body", dating to #197): GitHub rejects a REQUEST_CHANGES review event with an empty body, and under gh-aw v0.81.6 the safe-output flow posts the review event separately from the inline comments, so the comments do not make it non-empty. Observed live on the v1.5.0 behavior test (Khan/webapp#40793): the reviewer computed REQUEST_CHANGES correctly for two blocking findings, the submission failed with "Review body is required for REQUEST_CHANGES", and the PR was left with the inline comments as a COMMENTED review and no blocking verdict. The latent instruction survived this long because models usually filled the body anyway. The body's first line is now always `Changes requested — see inline comments.`, with the accountability section, note lines, and fingerprint stamp appended as before. diff --git a/workflows/review/lib/render-comment.test.ts b/workflows/review/lib/render-comment.test.ts index 8a72a825..b9ec7ffa 100644 --- a/workflows/review/lib/render-comment.test.ts +++ b/workflows/review/lib/render-comment.test.ts @@ -234,13 +234,12 @@ describe("renderReviewBody — one non-empty line per verdict (+ notes)", () => expect(body({event: "APPROVE", hasInlineComments: true})).toBe(""); }); - it("REQUEST_CHANGES with inline comments has an empty body", () => { - expect(body({event: "REQUEST_CHANGES", hasInlineComments: true})).toBe( - "", - ); - }); - - it("REQUEST_CHANGES without inline comments keeps the pointer line (degenerate case)", () => { + it("REQUEST_CHANGES always carries the pointer line (GitHub rejects an empty body)", () => { + // The inline comments post separately from the review event, so they + // never make it non-empty; an empty body loses the blocking verdict. + expect( + body({event: "REQUEST_CHANGES", hasInlineComments: true}), + ).toMatchInlineSnapshot(`"Changes requested — see inline comments."`); expect( body({event: "REQUEST_CHANGES", hasInlineComments: false}), ).toMatchInlineSnapshot(`"Changes requested — see inline comments."`); diff --git a/workflows/review/lib/render-comment.ts b/workflows/review/lib/render-comment.ts index 3a8b5d1b..83ac3052 100644 --- a/workflows/review/lib/render-comment.ts +++ b/workflows/review/lib/render-comment.ts @@ -222,12 +222,14 @@ const HOLD_UNSTUCK_LINES = [ * APPROVE/REQUEST_CHANGES, and renders a self-explanatory hold-for-human body * for the third event. * - * The body convention (matching `review.md`): when inline comments exist, the - * comments ARE the review, so the body stays empty; GitHub requires a non-empty - * body only when a review has no comments. A non-empty body therefore appears - * only for a comment-less review, for skipped-dimension notes (appended to - * every verdict, and forming the entire body when the head is empty), and for - * HOLD_FOR_HUMAN, which must always explain itself and how to proceed. + * The body convention (matching `review.md`): on APPROVE with inline comments + * the comments ARE the review, so the body stays empty; a REQUEST_CHANGES body + * is always non-empty, because GitHub rejects the event with an empty body and + * the safe-output flow posts the inline comments separately, so they do not + * make the event non-empty. Additional body text appears for a comment-less + * approval, for skipped-dimension notes (appended to every verdict, and + * forming the entire body when the head is empty), and for HOLD_FOR_HUMAN, + * which must always explain itself and how to proceed. */ export const renderReviewBody = (input: ReviewBodyInput): string => { let head: string; @@ -253,13 +255,10 @@ export const renderReviewBody = (input: ReviewBodyInput): string => { break; } case "REQUEST_CHANGES": - // A REQUEST_CHANGES verdict normally carries at least one blocking - // inline comment (the verdict follows from the posted labels), so - // its body is empty too; the pointer line covers only the - // degenerate comment-less case. - head = input.hasInlineComments - ? "" - : "Changes requested — see inline comments."; + // GitHub rejects a REQUEST_CHANGES review event with an empty + // body, and the inline comments post separately, so they never + // make the event non-empty: the pointer line is unconditional. + head = "Changes requested — see inline comments."; break; case "HOLD_FOR_HUMAN": head = diff --git a/workflows/review/review.md b/workflows/review/review.md index a8d424ed..da3400cb 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -1238,11 +1238,12 @@ succeeds. One call. The review body is NOT a status update — never say a review is "under way" or "completed". All specific feedback lives in the inline comments, and on approval -the risk summary and common patterns live in a separate PR comment (Step 7). When you -left at least one inline comment in Step 5, the inline comments ARE the review: -submit the verdict with an **empty** body (GitHub requires a non-empty body only when -a review has no comments). A non-empty body exists only to keep a comment-less review -submittable, or to carry a skipped-dimension note (below). +the risk summary and common patterns live in a separate PR comment (Step 7). On an +APPROVE with at least one inline comment, the inline comments ARE the review and +the body stays **empty**; a REQUEST_CHANGES body is **always non-empty** (GitHub +rejects the event otherwise — the inline comments post separately and do not make +it non-empty). Beyond those rules, body text exists only to keep a comment-less +approval submittable or to carry a skipped-dimension note (below). **If APPROVE:** @@ -1251,13 +1252,15 @@ submittable, or to carry a skipped-dimension note (below). - **If you left no inline comments**, submit the APPROVE event with the body set to exactly `Approved — no blocking issues found.` and nothing else. -**If REQUEST_CHANGES:** a REQUEST_CHANGES verdict carries at least one blocking -inline comment (the verdict follows from the comments you posted), so submit it with -an **empty** body. Only if no inline comment was posted (which should not happen), -keep the body to a single line: +**If REQUEST_CHANGES:** always submit the event with a non-empty body whose first +line is exactly: ``` Changes requested — see inline comments. ``` +GitHub REJECTS a REQUEST_CHANGES review event with an empty body (the safe-output +submission posts the event separately from the inline comments, so the comments do +not make it non-empty); an empty body here loses the blocking verdict entirely +while the inline comments post as a mere COMMENTED review. **Re-review accountability (either verdict; code-rendered).** When `threads.json` (Step 3 Phase 2) staged at least one unresolved bot thread this run, @@ -1301,8 +1304,8 @@ Also append here the re-review depth or tripwire note queued in Step 3, when the is one. These note lines, the code-rendered re-review accountability section, and the hidden fingerprint stamp below are the only text permitted beyond the verdict bodies above, and they apply to both APPROVE -and REQUEST_CHANGES, including the empty-body cases: when the body is otherwise -empty, they are the entire body. +and REQUEST_CHANGES, including the empty-body APPROVE case: when the body is +otherwise empty, they are the entire body. **The re-review fingerprint stamp (every submitted review; code-rendered).** Last, render this run's stamp with the verdict event you are about to submit: