diff --git a/.github/scripts/latest-review-decision-policy.test.mjs b/.github/scripts/latest-review-decision-policy.test.mjs index 0c45e78a8a..646187f25d 100644 --- a/.github/scripts/latest-review-decision-policy.test.mjs +++ b/.github/scripts/latest-review-decision-policy.test.mjs @@ -10,6 +10,7 @@ const reconcile = await readFile( new URL("../workflows/latest-review-decision-reconcile.yml", import.meta.url), "utf8", ); +const readme = await readFile(new URL("../../README.md", import.meta.url), "utf8"); test("the review event workflow is an unprivileged signal only", () => { assert.match(signal, /^\s{2}pull_request_review:\n\s{4}types: \[submitted\]/m); @@ -24,7 +25,6 @@ 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/); @@ -39,3 +39,11 @@ test("automatic reconciliation is serialized and historical PRs can be swept man assert.match(reconcile, /github\.event\.workflow_run\.conclusion == 'success'/); assert.match(reconcile, /github\.event\.workflow_run\.event == 'pull_request_review'/); }); + +test("the README documents the latest write-access review as the final decision", () => { + assert.match( + readme, + /쓰기 권한이 있는 리뷰어별 최신 `APPROVED`·`CHANGES_REQUESTED` 가운데 PR 전체에서 가장 늦은 판정을 최종 판정으로 사용한다/, + ); + assert.doesNotMatch(readme, /\/review-response/); +}); diff --git a/.github/scripts/reconcile-latest-review-decision.mjs b/.github/scripts/reconcile-latest-review-decision.mjs index 1cc785bf59..32c076fdeb 100644 --- a/.github/scripts/reconcile-latest-review-decision.mjs +++ b/.github/scripts/reconcile-latest-review-decision.mjs @@ -1,11 +1,9 @@ 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() === "") { @@ -26,19 +24,6 @@ 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)) { @@ -54,14 +39,18 @@ function normalizeDecisiveReview(review) { } const id = requiredPositiveInteger(review?.id, "review.id"); - const reviewer = normalizeLogin( + const reviewer = requiredString( review?.user?.login ?? review?.reviewer, "review.user.login", - ); - const { timestampText: submittedAt, timestamp: submittedTimestamp } = normalizeTimestamp( + ).toLowerCase(); + const submittedAt = requiredString( 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 }; } @@ -101,181 +90,7 @@ export function selectLatestDecisiveReviewsByReviewer(reviews) { return [...latestByReviewer.values()].sort(compareDecisiveReviews); } -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 = {}) { +export function buildDismissalPlan(reviews, permissionByReviewer) { if (!(permissionByReviewer instanceof Map)) { throw new Error("permissionByReviewer 는 Map 이어야 합니다."); } @@ -289,80 +104,20 @@ export function buildDismissalPlan(reviews, permissionByReviewer, context = {}) }); if (writeReviews.length === 0) { - return { - status: "no-write-review", - latestReview: null, - blockingReviews: [], - responseEvidence: [], - pendingReviewers: [], - }; + return { status: "no-write-review", latestReview: null, blockingReviews: [] }; } const latestReview = writeReviews.at(-1); if (latestReview.state !== "APPROVED") { - return { - status: "latest-changes-requested", - latestReview, - blockingReviews: [], - responseEvidence: [], - pendingReviewers: [], - }; + return { status: "latest-changes-requested", latestReview, blockingReviews: [] }; } const blockingReviews = writeReviews.filter((review) => review.state === "CHANGES_REQUESTED"); if (blockingReviews.length === 0) { - 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: "already-approved", latestReview, blockingReviews: [] }; } - return { - status: "dismiss", - latestReview, - blockingReviews, - responseEvidence, - pendingReviewers: [], - pullAuthor, - }; + return { status: "dismiss", latestReview, blockingReviews }; } export class GitHubClient { @@ -425,21 +180,9 @@ 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(); @@ -452,12 +195,7 @@ export async function planPullRequest(client, repository, pullNumber) { return { pullNumber: normalizedPullNumber, - ...buildDismissalPlan(reviews, permissionByReviewer, { - pullAuthor: pullRequest?.user?.login, - commits, - issueComments, - reviewComments, - }), + ...buildDismissalPlan(reviews, permissionByReviewer), }; } @@ -468,7 +206,6 @@ 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의 최종 승인이 올바르지 않습니다.`); @@ -483,27 +220,10 @@ 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 = [ - `작성자 @${pullAuthor}의 대응(${action.kind} ${action.id}) 뒤`, - `@${blockingReview.reviewer}와 @${approval.reviewer}가 모두 재검토했습니다.`, - `더 늦은 승인(review #${approval.id})을 최종 판정으로 사용합니다.`, + `@${approval.reviewer}의 더 늦은 승인(review #${approval.id})을 최종 판정으로 사용합니다.`, + "저장소 정책은 PR 전체에서 가장 최근의 APPROVED/CHANGES_REQUESTED 판정을 우선합니다.", ].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 221931c0d2..acc6303bc6 100644 --- a/.github/scripts/reconcile-latest-review-decision.test.mjs +++ b/.github/scripts/reconcile-latest-review-decision.test.mjs @@ -5,7 +5,6 @@ import { applyDismissalPlan, buildDismissalPlan, classifyRepositoryPermission, - collectAuthorActions, parseTargetPullNumber, reconcileLatestReviewDecisions, selectLatestDecisiveReviewsByReviewer, @@ -25,75 +24,19 @@ 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-26T03:00:00Z", + submittedAt: "2026-08-26T02: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, @@ -107,7 +50,7 @@ test("latest decisive review is selected separately for each reviewer", () => { id: 12, reviewer: "ALICE", state: "APPROVED", - submittedAt: "2026-08-26T03:30:00Z", + submittedAt: "2026-08-26T03:00:00Z", }), approvalByBob, ]; @@ -125,207 +68,46 @@ test("latest decisive review is selected separately for each reviewer", () => { ); }); -test("a later approval cannot dismiss an untouched change request", () => { +test("a later write-access approval dismisses another reviewer's blocking 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), [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"]); + assert.deepEqual(plan.blockingReviews.map(({ id }) => id), [10]); }); -test("a thread reply to one blocker does not satisfy another blocker", () => { +test("one later approval dismisses every older write-access blocking request", async () => { const changesByCarol = review({ - id: 30, - reviewer: "Carol", - state: "CHANGES_REQUESTED", - submittedAt: "2026-08-26T01:10:00Z", - }); - const secondChangesByCarol = review({ - id: 31, + id: 15, reviewer: "Carol", state: "CHANGES_REQUESTED", - submittedAt: "2026-08-26T02:10:00Z", + submittedAt: "2026-08-26T01:30:00Z", }); - const root = { - id: 100, - pull_request_review_id: 10, - created_at: "2026-08-26T00:59:00Z", - user: { login: "Alice" }, + const plan = { + pullNumber: 914, + ...buildDismissalPlan( + [changesByAlice, changesByCarol, approvalByBob], + new Map([["alice", "write"], ["bob", "maintain"], ["carol", "admin"]]), + ), }; - const reply = { - id: 101, - in_reply_to_id: 100, - created_at: "2026-08-26T01:30:00Z", - user: { login: "Author" }, + const calls = []; + const client = { + async request(path) { + calls.push(path); + return { state: "DISMISSED" }; + }, }; - 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"]); + const dismissed = await applyDismissalPlan(client, "Afternote/Afternote-FE", plan); + + assert.deepEqual(dismissed.map(({ id }) => id), [10, 15]); + assert.deepEqual(calls, [ + "/repos/Afternote/Afternote-FE/pulls/914/reviews/10/dismissals", + "/repos/Afternote/Afternote-FE/pulls/914/reviews/15/dismissals", + ]); }); test("a later change request still blocks after an earlier approval", () => { @@ -334,7 +116,7 @@ test("a later change request still blocks after an earlier approval", () => { id: 30, reviewer: "Alice", state: "CHANGES_REQUESTED", - submittedAt: "2026-08-26T04:00:00Z", + submittedAt: "2026-08-26T03:00:00Z", })], new Map([["alice", "admin"], ["bob", "write"]]), ); @@ -353,12 +135,18 @@ 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"]]), + new Map([["alice", "write"], ["carol", "write"]]), ); - assert.equal(plan.status, "already-approved"); - assert.deepEqual(plan.blockingReviews, []); + assert.equal(plan.status, "dismiss"); + assert.deepEqual(plan.blockingReviews.map(({ id }) => id), [12]); }); test("read-only approval never overrides a write-access change request", () => { @@ -372,19 +160,16 @@ 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-26T03:00:00Z"; + const submittedAt = "2026-08-26T01: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, "awaiting-reviewed-author-response"); + assert.equal(plan.status, "dismiss"); assert.equal(plan.latestReview.id, 101); }); @@ -406,11 +191,7 @@ test("only valid optional target pull numbers are accepted", () => { assert.throws(() => parseTargetPullNumber("abc"), /양의 정수/); }); -function executableDismissalPlan() { - return { pullNumber: 914, ...reviewedCommitPlan() }; -} - -test("dismissal calls the protected review endpoint and explains the reviewed author response", async () => { +test("dismissal calls the protected review endpoint and verifies the response", async () => { const calls = []; const client = { async request(path, options) { @@ -418,42 +199,47 @@ test("dismissal calls the protected review endpoint and explains the reviewed au 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", - executableDismissalPlan(), - ); + const dismissed = await applyDismissalPlan(client, "Afternote/Afternote-FE", plan); - assert.deepEqual(dismissed.map(({ id }) => id), [15]); + assert.deepEqual(dismissed.map(({ id }) => id), [10]); assert.equal( calls[0].path, - "/repos/Afternote/Afternote-FE/pulls/914/reviews/15/dismissals", + "/repos/Afternote/Afternote-FE/pulls/914/reviews/10/dismissals", ); assert.equal(calls[0].options.method, "PUT"); assert.equal(calls[0].options.body.event, "DISMISS"); - assert.match(calls[0].options.body.message, /작성자 @author/); - assert.match(calls[0].options.body.message, /@alice와 @bob/); + assert.match(calls[0].options.body.message, /@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", executableDismissalPlan()), + applyDismissalPlan(client, "Afternote/Afternote-FE", { + pullNumber: 914, + status: "dismiss", + latestReview: approvalByBob, + blockingReviews: [changesByAlice], + }), /DISMISSED가 아닙니다/, ); }); @@ -463,27 +249,15 @@ 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, secondChangesByAlice, approvalByBob]; + return [changesByAlice, approvalByBob]; } - if (path.includes("/pulls/1/commits")) { - return [commit()]; - } - 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 []; + throw new Error("second PR review lookup failed"); }, 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 a0b38c3163..270dfc1ece 100644 --- a/.github/workflows/latest-review-decision-reconcile.yml +++ b/.github/workflows/latest-review-decision-reconcile.yml @@ -28,7 +28,6 @@ jobs: timeout-minutes: 10 permissions: contents: read - issues: read pull-requests: write steps: - name: Clone trusted default branch diff --git a/README.md b/README.md index f90d84df52..9438b68d16 100644 --- a/README.md +++ b/README.md @@ -367,4 +367,5 @@ docker run --rm -v "$PWD":/workspace -w /workspace afternote-screenshot:latest \ - 현재 `develop`·`main` 머지에는 **승인 1건**이 필요하다. `main`은 모든 리뷰 스레드 해결도 필요하다. - 리뷰 결과는 GitHub의 `APPROVED`·`CHANGES_REQUESTED`·일반 코멘트로 표현한다. 별도 RCA prefix나 12시간 SLA는 두지 않는다. - [리뷰 적체 가드](.github/workflows/review-debt-guard.yml)는 응답을 기다리는 다른 PR이 남은 팀원의 새 PR을 닫을 수 있다. 각 PR은 팀원 한 명이 먼저 유효한 판정을 내리면 최초 미응답 목록에서 빠진다. -- 변경 요청에 대응한 뒤에는 실질 커밋, `/review-response` 코멘트 또는 해당 리뷰 스레드 답변으로 근거를 남기고 재검토를 받는다. 긴급 PR은 근거가 있을 때만 `review-debt-exempt` 라벨로 적체 가드를 우회한다. +- 쓰기 권한이 있는 리뷰어별 최신 `APPROVED`·`CHANGES_REQUESTED` 가운데 PR 전체에서 가장 늦은 판정을 최종 판정으로 사용한다. 가장 늦은 판정이 승인이면 더 오래된 변경 요청은 자동 해제되고, 승인 뒤에 새 변경 요청이 오면 다시 차단된다. +- 긴급 PR은 근거가 있을 때만 `review-debt-exempt` 라벨로 적체 가드를 우회한다.