diff --git a/.github/scripts/enforce-oldest-review-order.mjs b/.github/scripts/enforce-oldest-review-order.mjs index 68bb255d8c..8685ed0edc 100644 --- a/.github/scripts/enforce-oldest-review-order.mjs +++ b/.github/scripts/enforce-oldest-review-order.mjs @@ -186,16 +186,13 @@ export function selectOlderPullRequests(openPullRequests, currentPullRequest, re .sort(comparePullRequestsOldestFirst); } -/** - * review-debt-guard 와 같은 방식으로 리뷰어별 최신 결정만 남긴다. - * 누군가의 최신 결정이 CHANGES_REQUESTED 면 그중 가장 늦은 시각을 기준으로 삼는다. - */ +/** review-debt-guard 와 같이 PR 전체에서 가장 최근에 제출된 결정만 남긴다. */ export function analyzeDecisiveReviews(reviews) { if (!Array.isArray(reviews)) { throw new Error("리뷰 목록 응답이 배열이 아닙니다."); } - const latestByReviewer = new Map(); + let latest = null; for (const review of reviews) { const state = normalizeReviewState(review?.state); if (!DECISIVE_REVIEW_STATES.has(state)) { @@ -204,42 +201,37 @@ export function analyzeDecisiveReviews(reviews) { const reviewer = requiredString(review.user?.login, "review.user.login").toLowerCase(); const submittedAt = requiredString(review.submitted_at, "review.submitted_at"); const submittedTimestamp = parseTimestamp(submittedAt, "review.submitted_at"); - const previous = latestByReviewer.get(reviewer); - if (!previous || submittedTimestamp >= previous.submittedTimestamp) { - latestByReviewer.set(reviewer, { + if (!latest || submittedTimestamp >= latest.submittedTimestamp) { + latest = { reviewer, state, submittedAt, submittedTimestamp, - }); + }; } } - if (latestByReviewer.size === 0) { + if (!latest) { return { kind: "no-decisive-review", debt: true, blockedAt: null }; } - const outstanding = [...latestByReviewer.values()] - .filter((review) => review.state === "CHANGES_REQUESTED") - .sort((left, right) => left.submittedTimestamp - right.submittedTimestamp); - if (outstanding.length === 0) { + if (latest.state === "APPROVED") { return { kind: "resolved", debt: false, blockedAt: null }; } - const latest = outstanding.at(-1); return { kind: "changes-requested", debt: false, blockedAt: latest.submittedAt, blockedTimestamp: latest.submittedTimestamp, - outstandingReviews: outstanding, + outstandingReviews: [latest], }; } /** - * 변경요청을 낸 뒤 현재 다시 요청받은 리뷰어만 고른다. + * PR 전체의 최신 변경요청을 낸 뒤 현재 다시 요청받은 리뷰어만 고른다. * GitHub는 리뷰 제출 시 그 사람의 요청을 제거하므로, 같은 리뷰어가 현재 요청 목록에 - * 다시 있다면 마지막 결정 뒤 작성자가 명시적으로 재리뷰를 요청한 것이다. + * 다시 있다면 최신 결정 뒤 작성자가 명시적으로 재리뷰를 요청한 것이다. */ export function selectRequestedOutstandingReview(reviewState, requestedReviewers) { if (!Array.isArray(requestedReviewers)) { diff --git a/.github/scripts/enforce-oldest-review-order.test.mjs b/.github/scripts/enforce-oldest-review-order.test.mjs index 426e7123d2..5c8d864da2 100644 --- a/.github/scripts/enforce-oldest-review-order.test.mjs +++ b/.github/scripts/enforce-oldest-review-order.test.mjs @@ -173,10 +173,18 @@ test("결정 리뷰가 하나도 없으면 즉시 리뷰 빚이다", () => { assert.deepEqual(result, { kind: "no-decisive-review", debt: true, blockedAt: null }); }); -test("리뷰어별 최신 결정으로 미해소 CHANGES_REQUESTED를 찾는다", () => { +test("PR 전체의 최신 결정으로 현재 상태를 판단한다", () => { const resolved = analyzeDecisiveReviews([ - review({ state: "CHANGES_REQUESTED", submitted_at: "2026-08-01T00:00:00Z" }), - review({ state: "APPROVED", submitted_at: "2026-08-02T00:00:00Z" }), + review({ + state: "CHANGES_REQUESTED", + submitted_at: "2026-08-01T00:00:00Z", + user: { login: "alice" }, + }), + review({ + state: "APPROVED", + submitted_at: "2026-08-02T00:00:00Z", + user: { login: "bob" }, + }), ]); assert.equal(resolved.kind, "resolved"); @@ -204,10 +212,35 @@ test("변경요청을 낸 리뷰어에게 현재 재요청이 걸린 경우만 }), ]); - assert.equal(selectRequestedOutstandingReview(reviewState, ["ALICE"]).reviewer, "alice"); + assert.equal(selectRequestedOutstandingReview(reviewState, ["alice"]), null); + assert.equal(selectRequestedOutstandingReview(reviewState, ["BOB"]).reviewer, "bob"); assert.equal(selectRequestedOutstandingReview(reviewState, ["carol"]), null); }); +test("더 오래된 리뷰어의 요청은 이후 다른 변경요청을 리뷰 빚으로 되돌리지 않는다", () => { + const reviews = [ + review({ + state: "CHANGES_REQUESTED", + submitted_at: "2026-08-23T02:12:43Z", + user: { login: "Sadturtleman" }, + }), + review({ + state: "CHANGES_REQUESTED", + submitted_at: "2026-08-23T11:45:05Z", + user: { login: "1hyok" }, + }), + ]; + const commits = [commit({ + commit: { committer: { date: "2026-08-23T03:15:59Z" } }, + })]; + + const result = reviewDebtStatus(reviews, commits, ["Sadturtleman"]); + + assert.equal(result.debt, false); + assert.equal(result.reason, "changes-requested-not-rerequested"); + assert.equal(result.blockedAt, "2026-08-23T11:45:05Z"); +}); + test("변경요청 뒤 실질 커밋만 반영으로 세고 merge commit은 제외한다", () => { const blockedAt = "2026-08-02T00:00:00Z"; assert.equal( @@ -339,6 +372,41 @@ test("CHANGES_REQUESTED 뒤 같은 리뷰어에게 재요청하고 반영했으 assert.equal(result.oldestDebt.debtReason, "changes-requested-fixed-rerequested"); }); +test("과거 리뷰어 요청이 남아도 더 늦은 다른 변경요청 뒤에는 dismiss하지 않는다", async () => { + const client = fakeClient(({ apiPath }) => { + if (apiPath.includes("/permission")) return response({ permission: "maintain" }); + if (apiPath.includes("/pulls?state=open")) { + return response([pullRequest({ + number: 767, + requested_reviewers: [{ login: "Sadturtleman" }], + })]); + } + if (apiPath.includes("/pulls/767/reviews")) { + return response([ + review({ + state: "CHANGES_REQUESTED", + submitted_at: "2026-08-23T02:12:43Z", + user: { login: "Sadturtleman" }, + }), + review({ + state: "CHANGES_REQUESTED", + submitted_at: "2026-08-23T11:45:05Z", + user: { login: "1hyok" }, + }), + ]); + } + throw new Error(`최신 변경요청이 있는데 불필요한 API를 호출함: ${apiPath}`); + }); + + const result = await enforceOldestReviewOrder({ + event: reviewEvent(), repository: REPOSITORY, client, logger: silent, + }); + + assert.equal(result.status, "allowed"); + assert.equal(client.calls.some((call) => call.apiPath.includes("/commits")), false); + assert.equal(client.calls.some((call) => call.method !== "GET"), false); +}); + test("변경요청 뒤 수정했어도 재리뷰 요청이 없으면 허용한다", async () => { const client = fakeClient(({ apiPath }) => { if (apiPath.includes("/permission")) return response({ permission: "maintain" }); @@ -536,4 +604,6 @@ test("PR 생성 가드도 변경요청자의 명시적 재요청을 확인한다 assert.match(workflow, /requested_reviewers\[\]\.login/); assert.match(workflow, /blocked_by/); assert.match(workflow, /재리뷰 요청 없음/); + assert.match(workflow, /sort_by\(\.t\) \| last/); + assert.doesNotMatch(workflow, /group_by\(\.u/); }); diff --git a/.github/scripts/review-debt-policy.test.mjs b/.github/scripts/review-debt-policy.test.mjs index 73aa9ef060..de60440aaa 100644 --- a/.github/scripts/review-debt-policy.test.mjs +++ b/.github/scripts/review-debt-policy.test.mjs @@ -59,6 +59,8 @@ test("changes requested becomes debt only after explicit rerequest and a fix", ( assert.match(guard, /requested_reviewers\[\]\.login/); assert.match(guard, /blocked_by/); assert.match(guard, /명시적 재리뷰 요청 없음/); + assert.match(guard, /sort_by\(\.t\) \| last/); + assert.doesNotMatch(guard, /group_by\(\.u/); assert.match(guard, /\(\.parents \| length\) < 2/); assert.match(guard, /select\(\.commit\.committer\.date > \\"\$blocked_at\\"\)/); }); diff --git a/.github/workflows/review-debt-guard.yml b/.github/workflows/review-debt-guard.yml index 278c2e9931..b477e0a7d7 100644 --- a/.github/workflows/review-debt-guard.yml +++ b/.github/workflows/review-debt-guard.yml @@ -84,7 +84,8 @@ jobs: fi # 남은 것에서 «리뷰어의 공이 아닌» PR 을 걸러낸다. «누가» 봤는지는 따지지 - # 않는다 — 팀에서 한 사람이 보면 그 PR 의 리뷰는 채워진 것이다. + # 않는다 — 팀에서 한 사람이 더 늦은 판정을 내리면 그 판정이 PR 의 현재 + # 상태다. 더 오래된 다른 리뷰어의 변경요청을 다시 빚으로 살리지 않는다. # # 승인과 변경요청은 무게가 다르다. # APPROVED 머지 가능한 상태다. 그 뒤에 작성자가 손보는 것은 @@ -93,10 +94,11 @@ jobs: # 의무가 생기지는 않는다. 작성자가 같은 리뷰어에게 명시적으로 # 재요청해야 공이 돌아온다. # - # 그래서 빚은 두 종류다. (1) 아무도 판정을 내지 않은 PR, - # (2) 미해소 변경요청을 낸 리뷰어에게 재요청이 걸렸고 작성자가 이미 반영 - # 커밋을 올린 PR. 재요청이 없거나 변경요청 뒤 새 커밋이 없으면 빚이 아니다. - # 반영 여부는 병합 커밋(parents 2개 이상)을 뺀 실질 커밋으로만 센다. + # 그래서 빚은 두 종류다. (1) 아무도 판정을 내리지 않은 PR, + # (2) PR 전체의 최신 판정이 변경요청이고, 그 리뷰어에게 재요청이 걸렸으며, + # 그 판정 뒤 작성자가 반영 커밋을 올린 PR. 재요청이 없거나 최신 변경요청 뒤 + # 새 커밋이 없으면 빚이 아니다. 반영 여부는 병합 커밋(parents 2개 이상)을 + # 뺀 실질 커밋으로만 센다. debt_file=$(mktemp) reviews_file=$(mktemp) while IFS=$'\t' read -r pn created title requested_reviewers; do @@ -113,17 +115,17 @@ jobs: continue fi - # 리뷰어별 최신 판정이 CHANGES_REQUESTED 이면서, 현재 그 리뷰어에게 다시 - # 요청이 걸린 것 중 가장 늦은 판정. 리뷰 제출 시 기존 요청은 제거되므로 - # 이 교집합은 작성자가 마지막 판정 뒤 명시적으로 재요청했다는 뜻이다. + # PR 전체의 최신 판정만 본다. 그 판정이 CHANGES_REQUESTED 이면서 현재 같은 + # 리뷰어에게 요청이 다시 걸렸다면, 리뷰 제출 시 제거된 요청을 작성자가 + # 최신 판정 뒤 명시적으로 되살린 것이다. 이후 다른 리뷰어가 더 늦게 판정한 + # 경우에는 과거 리뷰어의 남은 요청으로 공을 되돌리지 않는다. blocked=$(jq -rs --arg active ",$requested_reviewers," ' - group_by(.u | ascii_downcase) | map(sort_by(.t) | last) - | map(.u |= ascii_downcase) - | map(select(.s == "CHANGES_REQUESTED")) - | map(select(.u as $reviewer - | $active | contains("," + $reviewer + ","))) - | sort_by(.t) - | if length == 0 then empty else last | [.u, .t] | @tsv end' \ + sort_by(.t) | last + | .u |= ascii_downcase + | select(.s == "CHANGES_REQUESTED") + | select(.u as $reviewer + | $active | contains("," + $reviewer + ",")) + | [.u, .t] | @tsv' \ "$reviews_file" 2>/dev/null || true) if [ -z "$blocked" ]; then @@ -156,7 +158,7 @@ jobs: { echo "## 🚧 리뷰 적체 가드 — 이 PR 을 닫습니다" echo - echo "**아무도 리뷰하지 않은** PR 이 **${count}건** 남아 있습니다." + echo "**리뷰 응답을 기다리는** PR 이 **${count}건** 남아 있습니다." echo echo "| PR | 열린 날 | 경과 | 제목 |" echo "|---|---|---|---|" @@ -167,7 +169,7 @@ jobs: echo "| #$n | $created | ${days}일 | $title |" done <<< "$debt" echo - echo "이 저장소는 아무도 응답하지 않은 PR 이 1건이라도 남아 있으면 새 PR 을 열 수 없습니다 (#1136)." + echo "이 저장소는 리뷰 응답을 기다리는 PR 이 1건이라도 남아 있으면 새 PR 을 열 수 없습니다 (#1136)." echo echo "각 PR 에 필요한 리뷰는 **한 건**입니다 — 누구든 먼저 판정을 내면 그 PR 은 모두의 목록에서 빠집니다. 승인 뒤 변경을 다시 볼 의무는 없고, **변경요청도 작성자가 같은 리뷰어에게 재요청한 뒤 반영 커밋을 올린 경우에만** 다시 리뷰어의 몫이 됩니다." echo