From e022887ba4b4c8ddb7806f5a19358b6c4c62a07b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EC=9D=BC=ED=98=81?= Date: Thu, 27 Aug 2026 16:16:15 +0900 Subject: [PATCH] =?UTF-8?q?fix(ci):=20=EC=9E=91=EC=84=B1=EC=9E=90=20?= =?UTF-8?q?=EB=8C=80=EC=9D=91=20=EB=92=A4=20=EC=9E=AC=EB=A6=AC=EB=B7=B0?= =?UTF-8?q?=EB=A5=BC=20=EC=9A=94=EA=B5=AC=ED=95=9C=EB=8B=A4?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../latest-review-decision-policy.test.mjs | 1 + .../reconcile-latest-review-decision.mjs | 310 +++++++++++++++- .../reconcile-latest-review-decision.test.mjs | 351 +++++++++++++++--- .../latest-review-decision-reconcile.yml | 1 + 4 files changed, 601 insertions(+), 62 deletions(-) diff --git a/.github/scripts/latest-review-decision-policy.test.mjs b/.github/scripts/latest-review-decision-policy.test.mjs index 369b8aeb15..0c45e78a8a 100644 --- a/.github/scripts/latest-review-decision-policy.test.mjs +++ b/.github/scripts/latest-review-decision-policy.test.mjs @@ -24,6 +24,7 @@ test("the trusted workflow reconciles from the default branch with minimal write assert.match(reconcile, /^\s{2}workflow_dispatch:/m); assert.match(reconcile, /^permissions: \{\}$/m); assert.match(reconcile, /^\s{6}contents: read$/m); + assert.match(reconcile, /^\s{6}issues: read$/m); assert.match(reconcile, /^\s{6}pull-requests: write$/m); assert.match(reconcile, /ref: \$\{\{ github\.event\.repository\.default_branch \}\}/); assert.match(reconcile, /persist-credentials: false/); diff --git a/.github/scripts/reconcile-latest-review-decision.mjs b/.github/scripts/reconcile-latest-review-decision.mjs index 32c076fdeb..1cc785bf59 100644 --- a/.github/scripts/reconcile-latest-review-decision.mjs +++ b/.github/scripts/reconcile-latest-review-decision.mjs @@ -1,9 +1,11 @@ import { pathToFileURL } from "node:url"; export const DECISIVE_REVIEW_STATES = new Set(["APPROVED", "CHANGES_REQUESTED"]); +export const REVIEW_RESPONSE_COMMAND = "/review-response"; const WRITE_PERMISSIONS = new Set(["admin", "maintain", "write"]); const NON_WRITE_PERMISSIONS = new Set(["triage", "read", "none"]); +const AUTHOR_ACTION_KINDS = new Set(["commit", "issue-comment", "review-reply"]); function requiredString(value, name) { if (typeof value !== "string" || value.trim() === "") { @@ -24,6 +26,19 @@ function normalizeReviewState(value) { return requiredString(value, "review.state").toUpperCase(); } +function normalizeLogin(value, name) { + return requiredString(value, name).toLowerCase(); +} + +function normalizeTimestamp(value, name) { + const timestampText = requiredString(value, name); + const timestamp = Date.parse(timestampText); + if (!Number.isFinite(timestamp)) { + throw new Error(`${name} 값이 올바른 시각이 아닙니다: ${timestampText}`); + } + return { timestampText, timestamp }; +} + function normalizeRepository(repository) { const value = requiredString(repository, "repository"); if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(value)) { @@ -39,18 +54,14 @@ function normalizeDecisiveReview(review) { } const id = requiredPositiveInteger(review?.id, "review.id"); - const reviewer = requiredString( + const reviewer = normalizeLogin( review?.user?.login ?? review?.reviewer, "review.user.login", - ).toLowerCase(); - const submittedAt = requiredString( + ); + const { timestampText: submittedAt, timestamp: submittedTimestamp } = normalizeTimestamp( review?.submitted_at ?? review?.submittedAt, "review.submitted_at", ); - const submittedTimestamp = Date.parse(submittedAt); - if (!Number.isFinite(submittedTimestamp)) { - throw new Error(`review.submitted_at 값이 올바른 시각이 아닙니다: ${submittedAt}`); - } return { id, reviewer, state, submittedAt, submittedTimestamp }; } @@ -90,7 +101,181 @@ export function selectLatestDecisiveReviewsByReviewer(reviews) { return [...latestByReviewer.values()].sort(compareDecisiveReviews); } -export function buildDismissalPlan(reviews, permissionByReviewer) { +function isSubstantiveReviewResponse(body) { + if (typeof body !== "string") { + return false; + } + const trimmed = body.trim(); + if (!trimmed.toLowerCase().startsWith(REVIEW_RESPONSE_COMMAND)) { + return false; + } + const remainder = trimmed.slice(REVIEW_RESPONSE_COMMAND.length); + return /^(?:[ \t]+|\r?\n)[\s\S]*\S$/.test(remainder); +} + +function normalizeAuthorAction({ kind, id, actedAt, reviewer = null }) { + const normalizedKind = requiredString(kind, "author action kind"); + if (!AUTHOR_ACTION_KINDS.has(normalizedKind)) { + throw new Error(`알 수 없는 author action kind 입니다: ${normalizedKind}`); + } + const normalizedId = requiredString( + typeof id === "number" ? String(id) : id, + "author action id", + ); + const { timestampText, timestamp } = normalizeTimestamp(actedAt, "author action actedAt"); + return { + kind: normalizedKind, + id: normalizedId, + actedAt: timestampText, + actedTimestamp: timestamp, + reviewer: reviewer === null ? null : normalizeLogin(reviewer, "author action reviewer"), + }; +} + +function compareAuthorActions(left, right) { + return left.actedTimestamp - right.actedTimestamp + || left.kind.localeCompare(right.kind) + || left.id.localeCompare(right.id); +} + +export function collectAuthorActions({ + pullAuthor, + reviews, + commits = [], + issueComments = [], + reviewComments = [], +}) { + const author = normalizeLogin(pullAuthor, "pull_request.user.login"); + for (const [name, value] of Object.entries({ commits, issueComments, reviewComments })) { + if (!Array.isArray(value)) { + throw new Error(`${name} 응답이 배열이 아닙니다.`); + } + } + + const decisiveReviews = reviews + .map((review) => normalizeDecisiveReview(review)) + .filter(Boolean); + const changeRequestById = new Map( + decisiveReviews + .filter((review) => review.state === "CHANGES_REQUESTED") + .map((review) => [review.id, review]), + ); + const actions = []; + + for (const commit of commits) { + const commitAuthors = [commit?.author?.login, commit?.committer?.login] + .filter((login) => typeof login === "string") + .map((login) => login.toLowerCase()); + if (!commitAuthors.includes(author)) { + continue; + } + if (!Array.isArray(commit?.parents)) { + throw new Error("commit.parents 응답이 배열이 아닙니다."); + } + if (commit.parents.length !== 1) { + continue; + } + actions.push(normalizeAuthorAction({ + kind: "commit", + id: requiredString(commit?.sha, "commit.sha"), + actedAt: commit?.commit?.committer?.date ?? commit?.commit?.author?.date, + })); + } + + for (const comment of issueComments) { + const commenter = comment?.user?.login; + if (typeof commenter !== "string" || commenter.toLowerCase() !== author) { + continue; + } + if (!isSubstantiveReviewResponse(comment?.body)) { + continue; + } + actions.push(normalizeAuthorAction({ + kind: "issue-comment", + id: requiredPositiveInteger(comment?.id, "issue comment.id"), + actedAt: comment?.created_at, + })); + } + + const reviewCommentById = new Map(); + for (const comment of reviewComments) { + reviewCommentById.set(requiredPositiveInteger(comment?.id, "review comment.id"), comment); + } + for (const comment of reviewComments) { + const commenter = comment?.user?.login; + if (typeof commenter !== "string" || commenter.toLowerCase() !== author) { + continue; + } + if (comment?.in_reply_to_id === undefined || comment?.in_reply_to_id === null) { + continue; + } + const rootCommentId = requiredPositiveInteger( + comment.in_reply_to_id, + "review comment.in_reply_to_id", + ); + const rootComment = reviewCommentById.get(rootCommentId); + if (!rootComment) { + throw new Error(`review comment #${comment.id}의 원본 댓글 #${rootCommentId}이 없습니다.`); + } + const rootReviewId = requiredPositiveInteger( + rootComment?.pull_request_review_id, + "review comment.pull_request_review_id", + ); + const changeRequest = changeRequestById.get(rootReviewId); + if (!changeRequest) { + continue; + } + actions.push(normalizeAuthorAction({ + kind: "review-reply", + id: requiredPositiveInteger(comment?.id, "review comment.id"), + actedAt: comment?.created_at, + reviewer: changeRequest.reviewer, + })); + } + + return actions.sort(compareAuthorActions); +} + +function findReviewedResponseEvidence({ + reviews, + blockingReview, + approval, + authorActions, +}) { + const reviewerReviews = reviews + .map((review) => normalizeDecisiveReview(review)) + .filter((review) => review && review.reviewer === blockingReview.reviewer) + .sort(compareDecisiveReviews); + const relevantActions = authorActions.filter((action) => + (action.reviewer === null || action.reviewer === blockingReview.reviewer) + && action.actedTimestamp < approval.submittedTimestamp); + const action = relevantActions.at(-1); + if (!action || action.actedTimestamp >= blockingReview.submittedTimestamp) { + return null; + } + + const priorReview = reviewerReviews + .filter((review) => review.submittedTimestamp < action.actedTimestamp) + .at(-1); + if (!priorReview || priorReview.state !== "CHANGES_REQUESTED") { + return null; + } + + const reviewsAfterResponse = reviewerReviews.filter((review) => + review.submittedTimestamp > action.actedTimestamp + && compareDecisiveReviews(review, approval) <= 0); + if (reviewsAfterResponse.length !== 1 || reviewsAfterResponse[0].id !== blockingReview.id) { + return null; + } + + return { + reviewer: blockingReview.reviewer, + priorBlockingReview: priorReview, + action, + }; +} + +export function buildDismissalPlan(reviews, permissionByReviewer, context = {}) { if (!(permissionByReviewer instanceof Map)) { throw new Error("permissionByReviewer 는 Map 이어야 합니다."); } @@ -104,20 +289,80 @@ export function buildDismissalPlan(reviews, permissionByReviewer) { }); if (writeReviews.length === 0) { - return { status: "no-write-review", latestReview: null, blockingReviews: [] }; + return { + status: "no-write-review", + latestReview: null, + blockingReviews: [], + responseEvidence: [], + pendingReviewers: [], + }; } const latestReview = writeReviews.at(-1); if (latestReview.state !== "APPROVED") { - return { status: "latest-changes-requested", latestReview, blockingReviews: [] }; + return { + status: "latest-changes-requested", + latestReview, + blockingReviews: [], + responseEvidence: [], + pendingReviewers: [], + }; } const blockingReviews = writeReviews.filter((review) => review.state === "CHANGES_REQUESTED"); if (blockingReviews.length === 0) { - return { status: "already-approved", latestReview, blockingReviews: [] }; + return { + status: "already-approved", + latestReview, + blockingReviews: [], + responseEvidence: [], + pendingReviewers: [], + }; + } + + const pullAuthor = normalizeLogin(context?.pullAuthor, "pull_request.user.login"); + const authorActions = collectAuthorActions({ + pullAuthor, + reviews, + commits: context?.commits, + issueComments: context?.issueComments, + reviewComments: context?.reviewComments, + }); + const responseEvidence = []; + const pendingReviewers = []; + for (const blockingReview of blockingReviews) { + const evidence = findReviewedResponseEvidence({ + reviews, + blockingReview, + approval: latestReview, + authorActions, + }); + if (evidence) { + responseEvidence.push(evidence); + } else { + pendingReviewers.push(blockingReview.reviewer); + } + } + + if (pendingReviewers.length > 0) { + return { + status: "awaiting-reviewed-author-response", + latestReview, + blockingReviews, + responseEvidence, + pendingReviewers, + pullAuthor, + }; } - return { status: "dismiss", latestReview, blockingReviews }; + return { + status: "dismiss", + latestReview, + blockingReviews, + responseEvidence, + pendingReviewers: [], + pullAuthor, + }; } export class GitHubClient { @@ -180,9 +425,21 @@ export class GitHubClient { export async function planPullRequest(client, repository, pullNumber) { const normalizedRepository = normalizeRepository(repository); const normalizedPullNumber = requiredPositiveInteger(pullNumber, "pull number"); + const pullRequest = await client.request( + `/repos/${normalizedRepository}/pulls/${normalizedPullNumber}`, + ); const reviews = await client.paginate( `/repos/${normalizedRepository}/pulls/${normalizedPullNumber}/reviews`, ); + const commits = await client.paginate( + `/repos/${normalizedRepository}/pulls/${normalizedPullNumber}/commits`, + ); + const issueComments = await client.paginate( + `/repos/${normalizedRepository}/issues/${normalizedPullNumber}/comments`, + ); + const reviewComments = await client.paginate( + `/repos/${normalizedRepository}/pulls/${normalizedPullNumber}/comments`, + ); const latestByReviewer = selectLatestDecisiveReviewsByReviewer(reviews); const permissionByReviewer = new Map(); @@ -195,7 +452,12 @@ export async function planPullRequest(client, repository, pullNumber) { return { pullNumber: normalizedPullNumber, - ...buildDismissalPlan(reviews, permissionByReviewer), + ...buildDismissalPlan(reviews, permissionByReviewer, { + pullAuthor: pullRequest?.user?.login, + commits, + issueComments, + reviewComments, + }), }; } @@ -206,6 +468,7 @@ export async function applyDismissalPlan(client, repository, plan) { } const pullNumber = requiredPositiveInteger(plan.pullNumber, "pull number"); + const pullAuthor = normalizeLogin(plan.pullAuthor, "pull_request.user.login"); const approval = normalizeDecisiveReview(plan.latestReview); if (!approval || approval.state !== "APPROVED") { throw new Error(`#${pullNumber} dismissal plan의 최종 승인이 올바르지 않습니다.`); @@ -220,10 +483,27 @@ export async function applyDismissalPlan(client, repository, plan) { if (compareDecisiveReviews(blockingReview, approval) >= 0) { throw new Error(`#${pullNumber} 최종 승인보다 늦은 변경요청은 dismiss할 수 없습니다.`); } + const evidence = plan.responseEvidence?.find( + (candidate) => candidate?.reviewer === blockingReview.reviewer, + ); + if (!evidence) { + throw new Error(`#${pullNumber} @${blockingReview.reviewer}의 검토된 작성자 대응 근거가 없습니다.`); + } + const priorBlockingReview = normalizeDecisiveReview(evidence?.priorBlockingReview); + const action = normalizeAuthorAction(evidence.action); + if (!priorBlockingReview || priorBlockingReview.state !== "CHANGES_REQUESTED") { + throw new Error(`#${pullNumber} @${blockingReview.reviewer}의 이전 변경요청 근거가 없습니다.`); + } + if (priorBlockingReview.reviewer !== blockingReview.reviewer + || !(priorBlockingReview.submittedTimestamp < action.actedTimestamp) + || !(action.actedTimestamp < blockingReview.submittedTimestamp)) { + throw new Error(`#${pullNumber} @${blockingReview.reviewer}의 작성자 대응·재리뷰 순서가 올바르지 않습니다.`); + } const message = [ - `@${approval.reviewer}의 더 늦은 승인(review #${approval.id})을 최종 판정으로 사용합니다.`, - "저장소 정책은 PR 전체에서 가장 최근의 APPROVED/CHANGES_REQUESTED 판정을 우선합니다.", + `작성자 @${pullAuthor}의 대응(${action.kind} ${action.id}) 뒤`, + `@${blockingReview.reviewer}와 @${approval.reviewer}가 모두 재검토했습니다.`, + `더 늦은 승인(review #${approval.id})을 최종 판정으로 사용합니다.`, ].join(" "); const response = await client.request( `/repos/${normalizedRepository}/pulls/${pullNumber}/reviews/${blockingReview.id}/dismissals`, diff --git a/.github/scripts/reconcile-latest-review-decision.test.mjs b/.github/scripts/reconcile-latest-review-decision.test.mjs index ba164a930c..221931c0d2 100644 --- a/.github/scripts/reconcile-latest-review-decision.test.mjs +++ b/.github/scripts/reconcile-latest-review-decision.test.mjs @@ -5,6 +5,7 @@ import { applyDismissalPlan, buildDismissalPlan, classifyRepositoryPermission, + collectAuthorActions, parseTargetPullNumber, reconcileLatestReviewDecisions, selectLatestDecisiveReviewsByReviewer, @@ -24,19 +25,75 @@ function review({ }; } +function commit({ + sha = "abc123", + author = "Author", + committedAt = "2026-08-26T01:30:00Z", + parentCount = 1, +} = {}) { + return { + sha, + author: { login: author }, + committer: { login: author }, + commit: { + author: { date: committedAt }, + committer: { date: committedAt }, + }, + parents: Array.from({ length: parentCount }, (_, index) => ({ sha: `parent-${index}` })), + }; +} + +function issueComment({ + id = 100, + author = "Author", + body = "/review-response\n요청한 변경 대신 현재 동작을 유지해야 하는 근거입니다.", + createdAt = "2026-08-26T01:30:00Z", +} = {}) { + return { + id, + body, + created_at: createdAt, + user: { login: author }, + }; +} + +function context(overrides = {}) { + return { + pullAuthor: "Author", + commits: [], + issueComments: [], + reviewComments: [], + ...overrides, + }; +} + const changesByAlice = review({ id: 10, reviewer: "Alice", state: "CHANGES_REQUESTED", submittedAt: "2026-08-26T01:00:00Z", }); +const secondChangesByAlice = review({ + id: 15, + reviewer: "Alice", + state: "CHANGES_REQUESTED", + submittedAt: "2026-08-26T02:00:00Z", +}); const approvalByBob = review({ id: 20, reviewer: "Bob", state: "APPROVED", - submittedAt: "2026-08-26T02:00:00Z", + submittedAt: "2026-08-26T03:00:00Z", }); +function reviewedCommitPlan() { + return buildDismissalPlan( + [changesByAlice, secondChangesByAlice, approvalByBob], + new Map([["alice", "write"], ["bob", "maintain"]]), + context({ commits: [commit()] }), + ); +} + test("latest decisive review is selected separately for each reviewer", () => { const reviews = [ changesByAlice, @@ -50,7 +107,7 @@ test("latest decisive review is selected separately for each reviewer", () => { id: 12, reviewer: "ALICE", state: "APPROVED", - submittedAt: "2026-08-26T03:00:00Z", + submittedAt: "2026-08-26T03:30:00Z", }), approvalByBob, ]; @@ -68,15 +125,207 @@ test("latest decisive review is selected separately for each reviewer", () => { ); }); -test("a later write-access approval dismisses another reviewer's blocking request", () => { +test("a later approval cannot dismiss an untouched change request", () => { const plan = buildDismissalPlan( [changesByAlice, approvalByBob], new Map([["alice", "write"], ["bob", "maintain"]]), + context(), ); + assert.equal(plan.status, "awaiting-reviewed-author-response"); + assert.deepEqual(plan.blockingReviews.map(({ id }) => id), [10]); + assert.deepEqual(plan.pendingReviewers, ["alice"]); +}); + +test("a non-merge author commit plus blocker and approver re-reviews permits dismissal", () => { + const plan = reviewedCommitPlan(); + assert.equal(plan.status, "dismiss"); assert.equal(plan.latestReview.id, 20); - assert.deepEqual(plan.blockingReviews.map(({ id }) => id), [10]); + assert.deepEqual(plan.blockingReviews.map(({ id }) => id), [15]); + assert.equal(plan.responseEvidence[0].priorBlockingReview.id, 10); + assert.equal(plan.responseEvidence[0].action.kind, "commit"); +}); + +test("a substantive author review-response comment can replace a code change", () => { + const plan = buildDismissalPlan( + [changesByAlice, secondChangesByAlice, approvalByBob], + new Map([["alice", "write"], ["bob", "write"]]), + context({ issueComments: [issueComment()] }), + ); + + assert.equal(plan.status, "dismiss"); + assert.equal(plan.responseEvidence[0].action.kind, "issue-comment"); +}); + +test("an author reply in the blocking review thread counts only for that reviewer", () => { + const root = { + id: 100, + pull_request_review_id: 10, + created_at: "2026-08-26T00:59:00Z", + user: { login: "Alice" }, + }; + const reply = { + id: 101, + in_reply_to_id: 100, + created_at: "2026-08-26T01:30:00Z", + user: { login: "Author" }, + }; + const plan = buildDismissalPlan( + [changesByAlice, secondChangesByAlice, approvalByBob], + new Map([["alice", "write"], ["bob", "write"]]), + context({ reviewComments: [root, reply] }), + ); + + assert.equal(plan.status, "dismiss"); + assert.equal(plan.responseEvidence[0].action.kind, "review-reply"); + assert.equal(plan.responseEvidence[0].action.reviewer, "alice"); +}); + +test("generic comments, empty commands, other users, and merge commits are not author responses", () => { + const reviews = [changesByAlice, secondChangesByAlice, approvalByBob]; + const actionInputs = { + commits: [commit({ parentCount: 2 }), commit({ author: "SomeoneElse" })], + issueComments: [ + issueComment({ id: 100, body: "반영하지 않겠습니다." }), + issueComment({ id: 101, body: "/review-response" }), + issueComment({ id: 102, author: "SomeoneElse" }), + ], + }; + const actions = collectAuthorActions({ + pullAuthor: "Author", + reviews, + reviewComments: [], + ...actionInputs, + }); + const plan = buildDismissalPlan( + reviews, + new Map([["alice", "write"], ["bob", "write"]]), + context(actionInputs), + ); + + assert.deepEqual(actions, []); + assert.equal(plan.status, "awaiting-reviewed-author-response"); +}); + +test("the blocker must re-review after the author response", () => { + const plan = buildDismissalPlan( + [changesByAlice, approvalByBob], + new Map([["alice", "write"], ["bob", "write"]]), + context({ commits: [commit({ committedAt: "2026-08-26T02:00:00Z" })] }), + ); + + assert.equal(plan.status, "awaiting-reviewed-author-response"); + assert.deepEqual(plan.pendingReviewers, ["alice"]); +}); + +test("the approver must review after the author response", () => { + const plan = buildDismissalPlan( + [changesByAlice, review({ + id: 20, + reviewer: "Bob", + state: "APPROVED", + submittedAt: "2026-08-26T01:20:00Z", + })], + new Map([["alice", "write"], ["bob", "write"]]), + context({ commits: [commit({ committedAt: "2026-08-26T01:30:00Z" })] }), + ); + + assert.equal(plan.status, "awaiting-reviewed-author-response"); +}); + +test("a fresh change request after a post-response approval starts a new blocked round", () => { + const approvalByAlice = review({ + id: 13, + reviewer: "Alice", + state: "APPROVED", + submittedAt: "2026-08-26T02:00:00Z", + }); + const freshChangesByAlice = review({ + id: 15, + reviewer: "Alice", + state: "CHANGES_REQUESTED", + submittedAt: "2026-08-26T02:30:00Z", + }); + const plan = buildDismissalPlan( + [changesByAlice, approvalByAlice, freshChangesByAlice, approvalByBob], + new Map([["alice", "write"], ["bob", "write"]]), + context({ commits: [commit()] }), + ); + + assert.equal(plan.status, "awaiting-reviewed-author-response"); + assert.deepEqual(plan.pendingReviewers, ["alice"]); +}); + +test("every active blocker must review a common author response", () => { + const changesByCarol = review({ + id: 30, + reviewer: "Carol", + state: "CHANGES_REQUESTED", + submittedAt: "2026-08-26T01:10:00Z", + }); + const secondChangesByCarol = review({ + id: 31, + reviewer: "Carol", + state: "CHANGES_REQUESTED", + submittedAt: "2026-08-26T02:10:00Z", + }); + const plan = buildDismissalPlan( + [ + changesByAlice, + changesByCarol, + secondChangesByAlice, + secondChangesByCarol, + approvalByBob, + ], + new Map([["alice", "write"], ["bob", "write"], ["carol", "write"]]), + context({ commits: [commit()] }), + ); + + assert.equal(plan.status, "dismiss"); + assert.deepEqual(plan.blockingReviews.map(({ reviewer }) => reviewer), ["alice", "carol"]); + assert.deepEqual(plan.responseEvidence.map(({ reviewer }) => reviewer), ["alice", "carol"]); +}); + +test("a thread reply to one blocker does not satisfy another blocker", () => { + const changesByCarol = review({ + id: 30, + reviewer: "Carol", + state: "CHANGES_REQUESTED", + submittedAt: "2026-08-26T01:10:00Z", + }); + const secondChangesByCarol = review({ + id: 31, + reviewer: "Carol", + state: "CHANGES_REQUESTED", + submittedAt: "2026-08-26T02:10:00Z", + }); + const root = { + id: 100, + pull_request_review_id: 10, + created_at: "2026-08-26T00:59:00Z", + user: { login: "Alice" }, + }; + const reply = { + id: 101, + in_reply_to_id: 100, + created_at: "2026-08-26T01:30:00Z", + user: { login: "Author" }, + }; + const plan = buildDismissalPlan( + [ + changesByAlice, + changesByCarol, + secondChangesByAlice, + secondChangesByCarol, + approvalByBob, + ], + new Map([["alice", "write"], ["bob", "write"], ["carol", "write"]]), + context({ reviewComments: [root, reply] }), + ); + + assert.equal(plan.status, "awaiting-reviewed-author-response"); + assert.deepEqual(plan.pendingReviewers, ["carol"]); }); test("a later change request still blocks after an earlier approval", () => { @@ -85,7 +334,7 @@ test("a later change request still blocks after an earlier approval", () => { id: 30, reviewer: "Alice", state: "CHANGES_REQUESTED", - submittedAt: "2026-08-26T03:00:00Z", + submittedAt: "2026-08-26T04:00:00Z", })], new Map([["alice", "admin"], ["bob", "write"]]), ); @@ -104,18 +353,12 @@ test("a reviewer's own later approval supersedes their earlier change request", state: "APPROVED", submittedAt: "2026-08-26T01:30:00Z", }), - review({ - id: 12, - reviewer: "Carol", - state: "CHANGES_REQUESTED", - submittedAt: "2026-08-26T01:15:00Z", - }), ], - new Map([["alice", "write"], ["carol", "write"]]), + new Map([["alice", "write"]]), ); - assert.equal(plan.status, "dismiss"); - assert.deepEqual(plan.blockingReviews.map(({ id }) => id), [12]); + assert.equal(plan.status, "already-approved"); + assert.deepEqual(plan.blockingReviews, []); }); test("read-only approval never overrides a write-access change request", () => { @@ -129,16 +372,19 @@ test("read-only approval never overrides a write-access change request", () => { }); test("same-second reviews use the larger review id as the later decision", () => { - const submittedAt = "2026-08-26T01:00:00Z"; + const submittedAt = "2026-08-26T03:00:00Z"; const plan = buildDismissalPlan( [ + changesByAlice, + secondChangesByAlice, review({ id: 100, reviewer: "Alice", state: "CHANGES_REQUESTED", submittedAt }), review({ id: 101, reviewer: "Bob", state: "APPROVED", submittedAt }), ], new Map([["alice", "write"], ["bob", "write"]]), + context({ commits: [commit()] }), ); - assert.equal(plan.status, "dismiss"); + assert.equal(plan.status, "awaiting-reviewed-author-response"); assert.equal(plan.latestReview.id, 101); }); @@ -160,7 +406,11 @@ test("only valid optional target pull numbers are accepted", () => { assert.throws(() => parseTargetPullNumber("abc"), /양의 정수/); }); -test("dismissal calls the protected review endpoint and verifies the response", async () => { +function executableDismissalPlan() { + return { pullNumber: 914, ...reviewedCommitPlan() }; +} + +test("dismissal calls the protected review endpoint and explains the reviewed author response", async () => { const calls = []; const client = { async request(path, options) { @@ -168,47 +418,42 @@ test("dismissal calls the protected review endpoint and verifies the response", return { state: "DISMISSED" }; }, }; - const plan = { - pullNumber: 914, - status: "dismiss", - latestReview: { - id: 20, - reviewer: "bob", - state: "APPROVED", - submittedAt: "2026-08-26T02:00:00Z", - submittedTimestamp: Date.parse("2026-08-26T02:00:00Z"), - }, - blockingReviews: [{ - id: 10, - reviewer: "alice", - state: "CHANGES_REQUESTED", - submittedAt: "2026-08-26T01:00:00Z", - submittedTimestamp: Date.parse("2026-08-26T01:00:00Z"), - }], - }; - const dismissed = await applyDismissalPlan(client, "Afternote/Afternote-FE", plan); + const dismissed = await applyDismissalPlan( + client, + "Afternote/Afternote-FE", + executableDismissalPlan(), + ); - assert.deepEqual(dismissed.map(({ id }) => id), [10]); + assert.deepEqual(dismissed.map(({ id }) => id), [15]); assert.equal( calls[0].path, - "/repos/Afternote/Afternote-FE/pulls/914/reviews/10/dismissals", + "/repos/Afternote/Afternote-FE/pulls/914/reviews/15/dismissals", ); assert.equal(calls[0].options.method, "PUT"); assert.equal(calls[0].options.body.event, "DISMISS"); - assert.match(calls[0].options.body.message, /@bob/); + assert.match(calls[0].options.body.message, /작성자 @author/); + assert.match(calls[0].options.body.message, /@alice와 @bob/); assert.match(calls[0].options.body.message, /review #20/); }); +test("dismissal fails closed when reviewed response evidence is missing", async () => { + const plan = executableDismissalPlan(); + plan.responseEvidence = []; + await assert.rejects( + applyDismissalPlan( + { async request() { return { state: "DISMISSED" }; } }, + "Afternote/Afternote-FE", + plan, + ), + /검토된 작성자 대응 근거가 없습니다/, + ); +}); + test("an unexpected dismiss response fails instead of reporting success", async () => { const client = { async request() { return { state: "CHANGES_REQUESTED" }; } }; await assert.rejects( - applyDismissalPlan(client, "Afternote/Afternote-FE", { - pullNumber: 914, - status: "dismiss", - latestReview: approvalByBob, - blockingReviews: [changesByAlice], - }), + applyDismissalPlan(client, "Afternote/Afternote-FE", executableDismissalPlan()), /DISMISSED가 아닙니다/, ); }); @@ -218,15 +463,27 @@ test("all pull requests are read and authorized before the first dismissal", asy const client = { async paginate(path) { if (path.includes("/pulls/1/reviews")) { - return [changesByAlice, approvalByBob]; + return [changesByAlice, secondChangesByAlice, approvalByBob]; + } + if (path.includes("/pulls/1/commits")) { + return [commit()]; } - throw new Error("second PR review lookup failed"); + if (path.includes("/issues/1/comments") || path.includes("/pulls/1/comments")) { + return []; + } + if (path.includes("/pulls/2/reviews")) { + throw new Error("second PR review lookup failed"); + } + return []; }, async request(path, options = {}) { if (options.method === "PUT") { writes.push(path); return { state: "DISMISSED" }; } + if (path.endsWith("/pulls/1") || path.endsWith("/pulls/2")) { + return { user: { login: "Author" } }; + } return { permission: "write" }; }, }; diff --git a/.github/workflows/latest-review-decision-reconcile.yml b/.github/workflows/latest-review-decision-reconcile.yml index 270dfc1ece..a0b38c3163 100644 --- a/.github/workflows/latest-review-decision-reconcile.yml +++ b/.github/workflows/latest-review-decision-reconcile.yml @@ -28,6 +28,7 @@ jobs: timeout-minutes: 10 permissions: contents: read + issues: read pull-requests: write steps: - name: Clone trusted default branch