Skip to content

fix(ci): 마지막 결정 리뷰만 리뷰 빚으로 판정한다 #1211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
1hyok merged 1 commit into from
Aug 26, 2026
Merged

fix(ci): 마지막 결정 리뷰만 리뷰 빚으로 판정한다 #1211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 10 additions & 18 deletions .github/scripts/enforce-oldest-review-order.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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)) {
Expand Down
78 changes: 74 additions & 4 deletions .github/scripts/enforce-oldest-review-order.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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" });
Expand Down Expand Up @@ -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/);
});
2 changes: 2 additions & 0 deletions .github/scripts/review-debt-policy.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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\\"\)/);
});
36 changes: 19 additions & 17 deletions .github/workflows/review-debt-guard.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ jobs:
fi

# 남은 것에서 «리뷰어의 공이 아닌» PR 을 걸러낸다. «누가» 봤는지는 따지지
# 않는다 — 팀에서 한 사람이 보면 그 PR 의 리뷰는 채워진 것이다.
# 않는다 — 팀에서 한 사람이 더 늦은 판정을 내리면 그 판정이 PR 의 현재
# 상태다. 더 오래된 다른 리뷰어의 변경요청을 다시 빚으로 살리지 않는다.
#
# 승인과 변경요청은 무게가 다르다.
# APPROVED 머지 가능한 상태다. 그 뒤에 작성자가 손보는 것은
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -156,7 +158,7 @@ jobs:
{
echo "## 🚧 리뷰 적체 가드 — 이 PR 을 닫습니다"
echo
echo "**아무도 리뷰하지 않은** PR 이 **${count}건** 남아 있습니다."
echo "**리뷰 응답을 기다리는** PR 이 **${count}건** 남아 있습니다."
echo
echo "| PR | 열린 날 | 경과 | 제목 |"
echo "|---|---|---|---|"
Expand All @@ -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
Expand Down
Loading