diff --git a/.github/scripts/review-debt-policy.test.mjs b/.github/scripts/review-debt-policy.test.mjs index de60440aaa..a6240bc5e8 100644 --- a/.github/scripts/review-debt-policy.test.mjs +++ b/.github/scripts/review-debt-policy.test.mjs @@ -3,6 +3,7 @@ import { readFile } from "node:fs/promises"; import test from "node:test"; const guard = await readFile(new URL("../workflows/review-debt-guard.yml", import.meta.url), "utf8"); +const requestAll = await readFile(new URL("../workflows/review-request-all.yml", import.meta.url), "utf8"); test("team membership is judged by real permission, not author_association", () => { // 웹훅 페이로드의 author_association 은 조직 멤버십이 비공개면 MEMBER 대신 @@ -64,3 +65,43 @@ test("changes requested becomes debt only after explicit rerequest and a fix", ( assert.match(guard, /\(\.parents \| length\) < 2/); assert.match(guard, /select\(\.commit\.committer\.date > \\"\$blocked_at\\"\)/); }); + +test("a fix delivered by a merge commit still counts as a response", () => { + // 비병합 커밋만 세면 base 를 끌어와 충돌을 풀며 반영한 PR 이 «미반영» 이 된다. + // #1316 은 리뷰가 지목한 파일이 실제로 바뀌고 작성자 응답 코멘트도 2건 달린 + // 채로 빚에서 빠져 있었다. 커밋과 함께 작성자의 응답을 본다 (#1450). + assert.match(guard, /issues\/\$pn\/comments/); + assert.match(guard, /pulls\/\$pn\/comments/); + assert.match(guard, /select\(\.user\.login == \\"\$pr_author\\"\)/); + assert.match(guard, /\[ "\$\{fixed:-0\}" -gt 0 \] \|\| \[ "\$responses" -gt 0 \]/); +}); + +test("counts are taken per item so paginated pull requests do not break the comparison", () => { + // --paginate 는 페이지마다 jq 를 돌린다. 페이지 단위로 length 를 뽑으면 커밋이 + // 100건을 넘는 PR 에서 숫자가 여러 줄로 나와 -gt 비교가 죽는다. + assert.doesNotMatch(guard, /\| length" 2>\/dev\/null/); + assert.match(guard, /\| wc -l \| tr -d ' '/); +}); + +test("rerequests are automated so silence cannot pass the guard", () => { + // 가드는 변경요청을 낸 리뷰어에게 요청이 다시 걸린 경우에만 빚으로 센다. 그 + // 되살리기를 작성자 손에 맡기면 아무도 걸지 않아 가드가 통째로 무력해진다 — + // 8/29 실측에서 반영까지 끝난 7건이 전원 «빚 아님» 이었다 (#1450). + assert.match(requestAll, /^\s*types: \[opened, ready_for_review, reopened, synchronize\]/m); + assert.match(requestAll, /^\s{2}rerequest:/m); + assert.match(requestAll, /github\.event\.action == 'synchronize'/); + assert.match(requestAll, /--add-reviewer "\$blocked"/); + // 기존 전원 요청은 반영 커밋마다 다시 돌지 않는다. + assert.match(requestAll, /github\.event\.action != 'synchronize'/); +}); + +test("the rerequest job and the guard judge by the same latest decision", () => { + // 한쪽만 «PR 전체의 최신 판정» 을 보면 자동 재요청이 빚으로 이어지지 않거나, + // 이미 승인된 PR 에 요청을 되살린다. + for (const wf of [guard, requestAll]) { + assert.match(wf, /sort_by\(\.t\) \| last/); + assert.match(wf, /CHANGES_REQUESTED/); + } + // 봇·fork 는 토큰이 read-only 라 요청을 걸 수 없다. + assert.match(requestAll, /\[ "\$HEAD_REPO" != "\$REPO" \]/); +}); diff --git a/.github/workflows/review-debt-guard.yml b/.github/workflows/review-debt-guard.yml index a0c2216525..a92a6289fa 100644 --- a/.github/workflows/review-debt-guard.yml +++ b/.github/workflows/review-debt-guard.yml @@ -79,7 +79,7 @@ jobs: # 아니므로 뺀다(review-request-all 도 draft 에는 요청을 걸지 않는다). if ! open_prs=$(gh api "repos/$REPO/pulls?state=open&per_page=100" --paginate \ --jq ".[] | select(.draft == false) | select(.user.login != \"$AUTHOR\") - | \"\(.number)\t\(.created_at[0:10])\t\(.title)\t\([.requested_reviewers[].login | ascii_downcase] | join(\",\"))\""); then + | \"\(.number)\t\(.created_at[0:10])\t\(.user.login)\t\([.requested_reviewers[].login | ascii_downcase] | join(\",\"))\t\(.title)\""); then echo "::error::열린 PR 조회에 실패했다 — 근거 없이 PR 을 닫지 않는다. 재실행할 것" exit 1 fi @@ -102,7 +102,7 @@ jobs: # 뺀 실질 커밋으로만 센다. debt_file=$(mktemp) reviews_file=$(mktemp) - while IFS=$'\t' read -r pn created title requested_reviewers; do + while IFS=$'\t' read -r pn created pr_author requested_reviewers title; do [ -n "$pn" ] || continue gh api "repos/$REPO/pulls/$pn/reviews" --paginate \ @@ -135,12 +135,34 @@ jobs: fi IFS=$'\t' read -r blocked_by blocked_at <<< "$blocked" - fixed=$(gh api "repos/$REPO/pulls/$pn/commits?per_page=100" --paginate \ - --jq "[.[] | select((.parents | length) < 2) - | select(.commit.committer.date > \"$blocked_at\")] | length" 2>/dev/null || true) - - if [ "${fixed:-0}" -gt 0 ]; then - echo "#$pn — @$blocked_by 재리뷰 요청 뒤 변경요청 반영(${fixed}커밋), 아직 미응답" + # 반영을 비병합 커밋으로만 세면 병합으로 반영한 PR 을 놓친다. base 를 + # 끌어와 충돌을 풀며 지적을 반영하면 그 뒤 커밋이 전부 parents 2개라 + # «미반영» 이 된다 — #1316 은 리뷰가 지목한 파일이 실제로 바뀌고 작성자 + # 응답 코멘트까지 2건 달린 채로 8/29 까지 빚에서 빠져 있었다(#1450). + # 병합 커밋의 first-parent diff 는 base 에서 들어온 변경을 통째로 담아 + # 반영분과 구별되지 않으므로, 커밋 대신 «작성자가 응답했는가» 를 함께 본다. + # + # 페이지마다 집계하면 100건이 넘는 PR 에서 숫자가 여러 줄로 나와 비교가 + # 깨진다. 건별로 뽑아 한 번에 센다. + fixed=$( { gh api "repos/$REPO/pulls/$pn/commits?per_page=100" --paginate \ + --jq ".[] | select((.parents | length) < 2) + | select(.commit.committer.date > \"$blocked_at\") | .sha" 2>/dev/null || true; } \ + | wc -l | tr -d ' ') + + said=$( { gh api "repos/$REPO/issues/$pn/comments?per_page=100" --paginate \ + --jq ".[] | select(.user.login == \"$pr_author\") + | select(.created_at > \"$blocked_at\") | .id" 2>/dev/null || true; } \ + | wc -l | tr -d ' ') + + answered=$( { gh api "repos/$REPO/pulls/$pn/comments?per_page=100" --paginate \ + --jq ".[] | select(.user.login == \"$pr_author\") + | select(.created_at > \"$blocked_at\") | .id" 2>/dev/null || true; } \ + | wc -l | tr -d ' ') + + responses=$(( ${said:-0} + ${answered:-0} )) + + if [ "${fixed:-0}" -gt 0 ] || [ "$responses" -gt 0 ]; then + echo "#$pn — @$blocked_by 재리뷰 요청 뒤 변경요청 반영(${fixed}커밋 · 응답 ${responses}건), 아직 미응답" printf '%s\t%s\t%s\n' "$pn" "$created" "$title" >> "$debt_file" else echo "#$pn — 변경요청 미반영, 공은 작성자에게" diff --git a/.github/workflows/review-request-all.yml b/.github/workflows/review-request-all.yml index e3b8e4de15..235c5d7d37 100644 --- a/.github/workflows/review-request-all.yml +++ b/.github/workflows/review-request-all.yml @@ -8,6 +8,13 @@ # "리뷰를 안 하면 새 PR 을 못 연다" 가 성립한다 — 요청을 손으로 걸던 때는 # 아예 걸지 않는 것으로 가드를 통과할 수 있었다. # +# 같은 구멍이 «변경요청 뒤» 에 그대로 남아 있었다(#1450). GitHub 은 변경요청 +# 리뷰를 제출하는 순간 그 리뷰어의 요청을 떼어 내는데, 가드는 그 리뷰어에게 +# 요청이 다시 걸린 경우에만 빚으로 센다. 되살리는 일이 작성자 손에 맡겨져 있어 +# 8/29 실측에서 반영까지 끝난 PR 7건이 전원 «빚 아님» 이었고, 그중 하나는 +# dependabot 이 작성자라 되살릴 주체 자체가 없었다. 반영 커밋이 올라오면 +# 재요청도 자동으로 건다. +# # 옛 Review Assign(.github/workflows/PRassign.yml)을 대체한다. 그쪽은 develop # 에서 파일만 지워졌을 뿐 워크플로 등록이 살아 있어, 파일이 남은 옛 브랜치에서 # 갈라진 PR 에는 지금도 구버전이 실행된다. 이 워크플로를 들이면서 함께 disable @@ -16,7 +23,7 @@ name: review-request-all on: pull_request: - types: [opened, ready_for_review, reopened] + types: [opened, ready_for_review, reopened, synchronize] permissions: pull-requests: write @@ -25,7 +32,7 @@ jobs: request: # draft 에는 걸지 않는다. 리뷰할 수 없는 PR 로 남의 미처리 건수를 늘리면 # review-debt-guard 가 그 사람의 새 PR 을 닫아 버린다. - if: github.event.pull_request.draft == false + if: github.event.pull_request.draft == false && github.event.action != 'synchronize' runs-on: ubuntu-latest timeout-minutes: 5 steps: @@ -71,3 +78,69 @@ jobs: echo "리뷰 요청: $targets" gh pr edit "$PR_NUMBER" --repo "$REPO" --add-reviewer "$targets" + + # 변경요청 뒤 반영 커밋이 올라오면 그 리뷰어에게 다시 요청을 건다(#1450). + # + # 판정 기준은 review-debt-guard 와 같아야 한다 — 여기서 거는 요청이 곧 그쪽의 + # 빚 성립 조건이므로, 한쪽만 «PR 전체의 최신 판정» 을 보면 자동 재요청이 빚으로 + # 이어지지 않거나 이미 승인된 PR 을 되살린다. + rerequest: + if: github.event.pull_request.draft == false && github.event.action == 'synchronize' + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: 변경요청 리뷰어에게 재요청 + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + AUTHOR: ${{ github.event.pull_request.user.login }} + HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} + run: | + set -euo pipefail + + # 봇 PR 은 건너뛴다. dependabot 이 연 PR 에서는 GITHUB_TOKEN 이 read-only 라 + # 요청을 걸지 못한다 — 시도하면 403 으로 죽을 뿐이다. 봇 PR 의 변경요청이 + # 빚이 되지 못하는 구멍은 이 워크플로로는 못 메운다(#1450 에 남겨 둔다). + case "$AUTHOR" in + *'[bot]'|dependabot|github-actions) echo "봇 PR — 건너뜀"; exit 0 ;; + esac + + # fork PR 도 토큰이 read-only 다. 팀원 여부를 author_association 으로 가르지 + # 않는 이유는 review-debt-guard 주석에 있다 — 비공개 멤버가 CONTRIBUTOR 로 + # 내려와 팀원 대부분이 조용히 빠진다. 실제 제약인 head 저장소로 가른다. + if [ "$HEAD_REPO" != "$REPO" ]; then + echo "fork PR($HEAD_REPO) — 토큰이 read-only 라 건너뜀" + exit 0 + fi + + # PR 전체의 최신 판정만 본다. 승인 뒤 올라온 커밋은 재요청 대상이 아니다. + # --paginate 는 페이지마다 jq 를 돌리므로 정렬은 모아서 한 번에 한다. + blocked=$( { gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" --paginate \ + --jq '.[] | select(.state == "APPROVED" or .state == "CHANGES_REQUESTED") + | {u: .user.login, s: .state, t: .submitted_at}' 2>/dev/null || true; } \ + | jq -rs 'sort_by(.t) | last // empty | select(.s == "CHANGES_REQUESTED") | .u') + + if [ -z "$blocked" ]; then + echo "최신 판정이 변경요청이 아님 — 재요청할 대상 없음" + exit 0 + fi + + # 자기 자신은 리뷰어가 될 수 없다(422). 작성자가 자기 PR 에 변경요청을 낼 + # 수는 없지만, 소유권이 옮겨진 PR 에서 그렇게 보일 수 있다. + if [ "$blocked" = "$AUTHOR" ]; then + echo "변경요청을 낸 사람이 작성자 본인 — 건너뜀" + exit 0 + fi + + # 이미 걸려 있으면 다시 걸지 않는다. synchronize 는 force-push 마다 온다. + current=$(gh api "repos/$REPO/pulls/$PR_NUMBER" \ + --jq '[.requested_reviewers[].login] | join(" ")') + blocked_lc=$(printf '%s' "$blocked" | tr '[:upper:]' '[:lower:]') + current_lc=$(printf '%s' "$current" | tr '[:upper:]' '[:lower:]') + case " $current_lc " in + *" $blocked_lc "*) echo "@$blocked 요청이 이미 걸려 있음 — 건너뜀"; exit 0 ;; + esac + + echo "변경요청 뒤 반영 커밋 — @$blocked 에게 재리뷰 요청" + gh pr edit "$PR_NUMBER" --repo "$REPO" --add-reviewer "$blocked"