fix: authenticate and reconcile approval delegations
This commit is contained in:
1 parent
60d26ac4ab
commit
3e9b6ad94e
9 files changed
+270
-80
No files matched your search
@@ -28,15 +28,15 @@ Production source means supported code files under `src/` in `packages/`, `apps/
|
||||
|
||||
Each reviewer contributes only the current `APPROVED` or `CHANGES_REQUESTED` decision that GitHub returns. A `DISMISSED` record clears that reviewer's standing decision, including earlier approvals. Comment-only and pending records do not replace a decision. Reviews from deleted accounts and reviewers without current repository access do not count. The workflow does not invalidate an approval by its review commit; the repository's native pull-request rules own stale-review and latest-push requirements.
|
||||
|
||||
The publisher runs when a pull request opens, synchronizes, reopens, becomes ready, becomes a draft, or is edited, including a base-branch change. It also runs when a PR conversation comment is created, edited, or deleted. Pull-request, review, and comment events share one concurrency group per PR. Review submissions, edits, and dismissals run the no-permission [`weighted-approval-review-event` workflow](../workflows/weighted-approval-review-event.yml); its validated run title supplies the pull-request number to the default-branch publisher. The publisher validates the current head, fetches every review and conversation comment, and resolves current repository permission before publishing the status. Permission changes take effect on the next subscribed event.
|
||||
The publisher runs when a pull request opens, synchronizes, reopens, becomes ready, becomes a draft, or is edited, including a base-branch change. Conversation comment events run the publisher only when the current or previous body contains `/delegate`. Closed or merged PRs and stale review heads are skipped before status writes or dependency setup; the workflow does not subscribe to master pushes. GitHub associates `workflow_run` publisher runs with the default branch even though approval statuses target the open PR head. Pull-request, review, and comment events share one concurrency group per PR. Review submissions, edits, and dismissals run the no-permission [`weighted-approval-review-event` workflow](../workflows/weighted-approval-review-event.yml); its validated run title supplies the pull-request number to the default-branch publisher. The publisher validates the current head, fetches every review and conversation comment, and resolves current repository permission before publishing the status. Permission changes take effect on the next subscribed event.
|
||||
|
||||
<a id="delegating-points"></a>
|
||||
|
||||
## Delegating points
|
||||
|
||||
Post `/delegate @username` as the entire text of a PR conversation comment to transfer your reviewer points to that user's effective `APPROVED` decision on this PR. For example, `/delegate @turtle1999` lets @turtle1999's approval count your points alongside their own. Until they approve, your points do not count, even if your own approval remains active. Creating or editing an active command dismisses the sender's existing `APPROVED` and `CHANGES_REQUESTED` reviews before requesting review from the recipient unless that user is already requested. Other requested reviewers remain unchanged. The command does not submit a review; drafts do not dismiss or request reviews. Comment-only and pending reviews cannot be dismissed. A failed dismissal publishes an error and prevents the review request; scores are refreshed after successful dismissal.
|
||||
Post `/delegate @username` as the entire text of a PR conversation comment to transfer your reviewer points to that user's effective `APPROVED` decision on this PR. For example, `/delegate @turtle1999` lets @turtle1999's approval count your points alongside their own. Until they approve, your points do not count, even if your own approval remains active. Each evaluation reconciles every active eligible command: it dismisses the sender's existing `APPROVED` and `CHANGES_REQUESTED` reviews, then requests review from recipients who have neither approved nor already been requested. This also handles commands posted on drafts once the PR becomes ready and commands whose original event was replaced in the concurrency queue. Other requested reviewers remain unchanged. The command does not submit a review; drafts do not dismiss or request reviews. Comment-only and pending reviews cannot be dismissed. A failed dismissal publishes an error and prevents review requests; scores are refreshed after successful dismissal. Request failures are logged without changing the published score, and subsequent evaluations retry outstanding requests.
|
||||
|
||||
Both accounts must currently have write or admin permission, and neither may be the PR author. Commands involving ineligible accounts are ignored. A sender's newest surviving syntactically valid command wins, in comment creation order; editing an older comment does not move it after newer comments. Submitting any review after delegation automatically takes the points back, including an approval, change request, or comment-only review. Pending reviews do not count; a later dismissal does not restore the delegation. When timestamps are equal, the review takes precedence. A new command after the review can delegate again; editing an older command cannot reactivate it. Post `/delegate @your-own-username` to restore your own review decision. Editing or deleting a command recomputes delegation from remaining comments, so deleting the latest command can restore an older one. Quoted commands, code blocks, review bodies, inline review comments, and commands mixed with other text do not count. Surrounding whitespace is allowed; account matching is case-insensitive.
|
||||
Both accounts must currently have write or admin permission, and neither may be the PR author. Commands involving ineligible accounts are ignored. A sender's newest surviving eligible, author-authorized command wins, in comment creation order; editing an older comment does not move it after newer comments. Submitting any review after delegation automatically takes the points back, including an approval, change request, or comment-only review. Submitting an inline diff comment with “Add single comment” also creates a comment-only review and reclaims the points. Pending reviews do not count; a later dismissal does not restore the delegation. When timestamps are equal, the review takes precedence. A new command after the review can delegate again; editing an older command cannot reactivate it. Post `/delegate @your-own-username` to restore your own review decision. Editing or deleting a command recomputes delegation from remaining comments, so deleting the latest command can restore an older one. Quoted commands, code blocks, review bodies, inline review comments, and commands mixed with other text do not count. Surrounding whitespace is allowed; account matching is case-insensitive. An edited command is accepted only when GitHub identifies the original author as its latest editor; edits by other accounts cannot delegate or cancel that author’s points.
|
||||
|
||||
Each sender contributes at most once, using their own fixed weight or production-line ownership. A delegate can receive points from multiple senders, but can transfer only their own points: delegated points are not forwarded through another delegation. Author credit cannot be delegated. Other reviewers' effective `CHANGES_REQUESTED` decisions still block the PR. The sender's old reviews are dismissed through GitHub, so their old change request stops blocking only after dismissal succeeds. That dismissal retains the original submission timestamp and does not cancel delegation; only a review submitted by the sender at or after the command automatically reclaims their points. Dismissing or replacing the delegate's approval removes the counted points but leaves the delegation active for the delegate's next approval. Logs identify each counted score owner and their delegate. Comment history failures or the 3,000-record limit fail evaluation instead of accepting a partial history.
|
||||
|
||||
@@ -44,7 +44,7 @@ Each sender contributes at most once, using their own fixed weight or production
|
||||
|
||||
## Security
|
||||
|
||||
All actions in the status-writing job are pinned to commit SHAs. The job checks out only the repository default branch. It does not check out or execute pull-request code and does not use repository secrets. Only when there is no blocker, reviewer points plus author credit are insufficient, and an approval has the policy’s default weight, it fetches complete history using the job token, passes Git objects to the trusted classifier as data, and resolves commit authors in batches of 50. Fetch credentials exist only in the Git child environment. Missing history, parsing failures, or incomplete author queries fail evaluation rather than producing a partial score. The review-event workflow has no `GITHUB_TOKEN` permissions and passes only a decimal pull-request number in its run title. The publisher accepts only successful `pull_request_review` runs from the review-event workflow file, identified by `workflow_run.path`; GitHub can populate `workflow_run.name` with the expanded run title. The publisher rejects an invalid run title and a number that does not resolve to the workflow run's current pull-request head. Pull-request reviews and comments are treated as API data and escaped in logs. Pull-request write permission allows dismissing the validated sender's prior decision reviews and requesting the delegate as a reviewer. Only a created or edited comment that is still the active eligible command can request review; deletion, review events, and superseded or revoked commands cannot. A failed dismissal or request fails the publisher and writes an error status.
|
||||
All actions in the status-writing job are pinned to commit SHAs. The job checks out only the repository default branch. It does not check out or execute pull-request code and does not use repository secrets. Only when there is no blocker, reviewer points plus author credit are insufficient, and an approval has the policy’s default weight, it fetches complete history using the job token, passes Git objects to the trusted classifier as data, and resolves commit authors in batches of 50. Fetch credentials exist only in the Git child environment. Missing history, parsing failures, or incomplete author queries fail evaluation rather than producing a partial score. The review-event workflow has no `GITHUB_TOKEN` permissions and passes only a decimal pull-request number in its run title. The publisher accepts only successful `pull_request_review` runs from the review-event workflow file, identified by `workflow_run.path`; GitHub can populate `workflow_run.name` with the expanded run title. The publisher rejects an invalid run title and a number that does not resolve to the workflow run's current pull-request head. Pull-request reviews and comments are treated as API data and escaped in logs. All publisher triggers and steps, including dependency setup, have pull-request write permission; the policy uses it to dismiss eligible senders' prior decision reviews and request recipients. Every evaluation verifies command authors and latest editors by immutable account ID through GraphQL, matching the body to the REST comment history before counting points or acting. Missing or inconsistent editor metadata fails evaluation. Non-author edits are ignored. Only active eligible delegations are reconciled; superseded or revoked commands have no effects. Dismissal failures publish an error; request failures remain separate from the approval decision.
|
||||
|
||||
Approval policy changes take effect only after they merge into the default branch. This prevents an untrusted pull request from changing the program or policy for its own run.
|
||||
|
||||
@@ -52,7 +52,7 @@ Approval policy changes take effect only after they merge into the default branc
|
||||
|
||||
## Verification
|
||||
|
||||
Run `pnpm run test:approval-policy` for policy parsing, effective review decisions, review-event validation, merged-history pagination and account matching, lazy author credit, permission filtering, weighted scoring, blockers, drafts, status publication, and API failures. [Workflow tests](../../scripts/ci-workflow.spec.ts) pin the trusted checkout, no-permission review handoff, permissions, events, and commands. The repository gate graph runs the approval policy and workflow tests in CI. The Python SDK job runs `uv run --python 3.10 --with-requirements .github/review-ownership/requirements.txt python -m unittest discover -s .github/review-ownership -p 'test_*.py'` for real Git histories, lexers, renames, shallow-history rejection, and the publisher’s fetch/analysis integration.
|
||||
Run `pnpm run test:approval-policy` for policy parsing, effective review decisions, review-event validation, merged-history pagination and account matching, lazy author credit, permission filtering, weighted scoring, delegation conservation and revocation, editor authorization, comment pagination, dismissal and review-request reconciliation, blockers, drafts, closed-PR skips, status publication, and API failures. [Workflow tests](../../scripts/ci-workflow.spec.ts) pin the trusted checkout, no-permission review handoff, permissions, events, and commands. The repository gate graph runs the approval policy and workflow tests in CI. The Python SDK job runs `uv run --python 3.10 --with-requirements .github/review-ownership/requirements.txt python -m unittest discover -s .github/review-ownership -p 'test_*.py'` for real Git histories, lexers, renames, shallow-history rejection, and the publisher’s fetch/analysis integration.
|
||||
|
||||
<a id="dev-note"></a>
|
||||
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
#!/usr/bin/env node
|
||||
|
||||
import { readFileSync } from 'node:fs'
|
||||
import { appendFileSync, readFileSync } from 'node:fs'
|
||||
import process from 'node:process'
|
||||
import { pathToFileURL } from 'node:url'
|
||||
|
||||
@@ -132,22 +132,62 @@ async function listRecords(api, path, subject) {
|
||||
}
|
||||
}
|
||||
|
||||
// The API orders comments by creation ID; edits do not reorder commands.
|
||||
function effectiveDelegations(comments, reviews) {
|
||||
const delegations = new Map()
|
||||
async function delegationCommands(api, comments) {
|
||||
const candidates = []
|
||||
for (const comment of comments) {
|
||||
if (!isRecord(comment)) throw new Error('pull-request comment is not an object')
|
||||
if (comment.user === null) continue
|
||||
if (typeof comment.body !== 'string') throw new Error('pull-request comment has no body')
|
||||
const match = /^\/delegate @([^\s]+)$/u.exec(comment.body.trim())
|
||||
if (!match || !LOGIN.test(match[1])) continue
|
||||
const login = validateLogin(comment.user?.login, 'delegation author').toLowerCase()
|
||||
const delegate = match[1].toLowerCase()
|
||||
if (login === delegate) delegations.delete(login)
|
||||
else {
|
||||
if (!Number.isSafeInteger(comment.id) || comment.id <= 0) throw new Error('delegation comment has no valid ID')
|
||||
delegations.set(login, { delegate, commentId: comment.id, createdAt: timestamp(comment.created_at, 'delegation comment') })
|
||||
const login = validateLogin(comment.user?.login, 'delegation author')
|
||||
if (!Number.isSafeInteger(comment.id) || comment.id <= 0) throw new Error('delegation comment has no valid ID')
|
||||
if (!comment.node_id || !comment.user.node_id) throw new Error('delegation comment has no account or node ID')
|
||||
candidates.push({ comment, login, delegate: match[1], createdAt: timestamp(comment.created_at, 'delegation comment') })
|
||||
}
|
||||
const commands = []
|
||||
for (let offset = 0; offset < candidates.length; offset += PAGE_SIZE) {
|
||||
const batch = candidates.slice(offset, offset + PAGE_SIZE)
|
||||
const response = await api('/graphql', {
|
||||
method: 'POST',
|
||||
body: {
|
||||
query: `query($ids: [ID!]!) {
|
||||
nodes(ids: $ids) { ... on IssueComment {
|
||||
id body createdAt lastEditedAt
|
||||
author { ... on Node { id } }
|
||||
editor { ... on Node { id } }
|
||||
} }
|
||||
}`,
|
||||
variables: { ids: batch.map(({ comment }) => comment.node_id) },
|
||||
},
|
||||
})
|
||||
const nodes = response?.data?.nodes
|
||||
if (response?.errors?.length || !Array.isArray(nodes) || nodes.length !== batch.length) {
|
||||
throw new Error('delegation comment editor history is incomplete')
|
||||
}
|
||||
for (const [index, command] of batch.entries()) {
|
||||
const node = nodes[index]
|
||||
const { comment } = command
|
||||
if (node?.id !== comment.node_id || node.body !== comment.body || node.createdAt !== comment.created_at
|
||||
|| node.author?.id !== comment.user.node_id || node.lastEditedAt === undefined) {
|
||||
throw new Error('delegation comment changed or editor history is incomplete')
|
||||
}
|
||||
// Other writers can edit a comment without changing its original author.
|
||||
if (node.lastEditedAt !== null && node.editor?.id !== node.author.id) continue
|
||||
commands.push(command)
|
||||
}
|
||||
}
|
||||
return commands
|
||||
}
|
||||
|
||||
// Reviews must first pass effectiveReviewDecisions; commands retain creation order after edits.
|
||||
function effectiveDelegations(commands, reviews, writers) {
|
||||
const delegations = new Map()
|
||||
for (const { login, delegate, comment, createdAt } of commands) {
|
||||
const key = login.toLowerCase()
|
||||
if (!writers.has(key) || !writers.has(delegate.toLowerCase())) continue
|
||||
if (key === delegate.toLowerCase()) delegations.delete(key)
|
||||
else delegations.set(key, { delegate: delegate.toLowerCase(), commentId: comment.id, createdAt })
|
||||
}
|
||||
for (const review of reviews) {
|
||||
if (review.user === null || review.state.toUpperCase() === 'PENDING') continue
|
||||
@@ -169,7 +209,7 @@ function timestamp(value, subject) {
|
||||
/**
|
||||
* Evaluate approval points from current reviews and repository permissions.
|
||||
* @param {{event: unknown, policySource: string, api: (path: string, options?: {method?: string, body?: unknown}) => Promise<unknown>, getOwnership?: typeof productionOwnership, getMergedCount?: typeof countMergedAuthorPulls}} options Runtime inputs.
|
||||
* @returns {Promise<{pull: {repository: string, number: number, headSha: string}, state: 'pending' | 'success', description: string, points: number, authorCredit: {mergedCount: number, points: number} | null, requiredPoints: number, approvals: Array<{login: string, points: number, delegatedTo?: string, ownership?: {ownedLines: number, totalLines: number}}>, delegations: Array<{login: string, delegatedTo: string, commentId: number, reviewIds: number[]}>, blockers: string[], ignoredReviewers: string[]}>} Approval decision and status payload fields; approval login owns the points, delegatedTo supplies its decision, delegations contains eligible active commands and their superseded decision review IDs, and null author credit means history was not evaluated.
|
||||
* @returns {Promise<{pull: {repository: string, number: number, headSha: string}, state: 'pending' | 'success', description: string, points: number, authorCredit: {mergedCount: number, points: number} | null, requiredPoints: number, approvals: Array<{login: string, points: number, delegatedTo?: string, ownership?: {ownedLines: number, totalLines: number}}>, delegations: Array<{login: string, delegatedTo: string, commentId: number, reviewIds: number[], approved: boolean}>, blockers: string[], ignoredReviewers: string[]}>} Approval decision and status payload fields; approval login owns the points, delegatedTo supplies its decision, delegations contains eligible active commands and their superseded decision review IDs, and null author credit means history was not evaluated.
|
||||
*/
|
||||
export async function evaluateApproval({ event, policySource, api, getOwnership = productionOwnership, getMergedCount = countMergedAuthorPulls }) {
|
||||
const pull = pullRequestFromEvent(event)
|
||||
@@ -182,11 +222,11 @@ export async function evaluateApproval({ event, policySource, api, getOwnership
|
||||
const decisions = effectiveReviewDecisions(reviews)
|
||||
.filter(({ login }) => login.toLowerCase() !== pull.author.toLowerCase())
|
||||
const comments = await listRecords(api, `/repos/${pull.repository}/issues/${pull.number}/comments`, 'pull-request comments')
|
||||
const delegations = effectiveDelegations(comments, reviews)
|
||||
const commands = await delegationCommands(api, comments)
|
||||
const participants = new Map(decisions.map(({ login }) => [login.toLowerCase(), login]))
|
||||
for (const [login, { delegate }] of delegations) {
|
||||
participants.set(login, participants.get(login) ?? login)
|
||||
participants.set(delegate, participants.get(delegate) ?? delegate)
|
||||
for (const { login, delegate } of commands) {
|
||||
participants.set(login.toLowerCase(), participants.get(login.toLowerCase()) ?? login)
|
||||
participants.set(delegate.toLowerCase(), participants.get(delegate.toLowerCase()) ?? delegate)
|
||||
}
|
||||
participants.delete(pull.author.toLowerCase())
|
||||
const writers = new Set()
|
||||
@@ -196,6 +236,7 @@ export async function evaluateApproval({ event, policySource, api, getOwnership
|
||||
if (WRITABLE_PERMISSIONS.has(permission)) writers.add(key)
|
||||
else ignoredReviewers.push(login)
|
||||
}
|
||||
const delegations = effectiveDelegations(commands, reviews, writers)
|
||||
const approvals = []
|
||||
const activeDelegations = []
|
||||
const blockers = decisions.filter(({ login, state }) => writers.has(login.toLowerCase()) && state === 'CHANGES_REQUESTED')
|
||||
@@ -206,7 +247,7 @@ export async function evaluateApproval({ event, policySource, api, getOwnership
|
||||
const target = delegation?.delegate
|
||||
const delegatedTo = writers.has(target) ? target : undefined
|
||||
if (delegatedTo) activeDelegations.push({
|
||||
login: participants.get(key), delegatedTo: participants.get(delegatedTo), commentId: delegation.commentId,
|
||||
login: participants.get(key), delegatedTo: participants.get(delegatedTo), commentId: delegation.commentId, approved: approved.has(delegatedTo),
|
||||
reviewIds: reviews.filter(review => review.user?.login.toLowerCase() === key
|
||||
&& ['APPROVED', 'CHANGES_REQUESTED'].includes(review.state.toUpperCase()))
|
||||
.map(review => positiveInteger(review.id, 'delegated review ID')),
|
||||
@@ -274,8 +315,7 @@ export async function runApprovalCheck({ event, policySource, api, runUrl, getOw
|
||||
let result
|
||||
try {
|
||||
result = await evaluateApproval({ event, policySource, api, getOwnership, getMergedCount })
|
||||
const delegation = activeCommentDelegation(event, result)
|
||||
if (delegation?.reviewIds.length) {
|
||||
for (const delegation of result.delegations) {
|
||||
for (const reviewId of delegation.reviewIds) {
|
||||
const dismissed = await api(`/repos/${pull.repository}/pulls/${pull.number}/reviews/${reviewId}/dismissals`, {
|
||||
method: 'PUT',
|
||||
@@ -286,9 +326,10 @@ export async function runApprovalCheck({ event, policySource, api, runUrl, getOw
|
||||
}
|
||||
write(`Dismissed @${delegation.login}'s review ${reviewId} for delegation to @${delegation.delegatedTo}.`)
|
||||
}
|
||||
}
|
||||
if (result.delegations.some(({ reviewIds }) => reviewIds.length)) {
|
||||
result = await evaluateApproval({ event, policySource, api, getOwnership, getMergedCount })
|
||||
}
|
||||
await requestDelegatedReview(event, result, api, write)
|
||||
} catch (error) {
|
||||
await publishStatus(api, pull, 'error', 'Approval evaluation failed.', runUrl)
|
||||
throw error
|
||||
@@ -303,6 +344,11 @@ export async function runApprovalCheck({ event, policySource, api, runUrl, getOw
|
||||
writeList(write, 'Ignored reviewers without write access', result.ignoredReviewers.map(login => `@${login}`))
|
||||
await publishStatus(api, pull, result.state, result.description, runUrl)
|
||||
write(`Published ${JSON.stringify(STATUS_CONTEXT)} status ${JSON.stringify(result.state)}.`)
|
||||
try {
|
||||
await requestDelegatedReviews(result, api, write)
|
||||
} catch (error) {
|
||||
write(`Review request failed; approval score is unchanged and the next evaluation will retry: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`)
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
@@ -320,7 +366,7 @@ export async function publishApprovalPhase({ event, api, runUrl, phase }) {
|
||||
/**
|
||||
* Resolve the reviewed pull request from a completed run of the review-event workflow file.
|
||||
* @param {{event: unknown, api: (path: string, options?: {method?: string, body?: unknown}) => Promise<unknown>}} options Trusted workflow inputs.
|
||||
* @returns {Promise<Record<string, unknown> | null>} Event with a current pull request, or null after the pull-request head changes.
|
||||
* @returns {Promise<Record<string, unknown> | null>} Event with a current pull request, or null after the PR closes or its head changes.
|
||||
*/
|
||||
export async function approvalEventFromWorkflowRun({ event, api }) {
|
||||
const repository = repositoryFromEvent(event)
|
||||
@@ -341,7 +387,8 @@ export async function approvalEventFromWorkflowRun({ event, api }) {
|
||||
}
|
||||
const pull = await api(`/repos/${repository}/pulls/${pullNumber}`)
|
||||
if (!isRecord(pull) || !isRecord(pull.head)) throw new Error(`pull request #${pullNumber} response is invalid`)
|
||||
if (pull.head.sha !== expectedHeadSha) return null
|
||||
if (!['open', 'closed'].includes(pull.state)) throw new Error('pull request has an invalid state')
|
||||
if (pull.state === 'closed' || pull.head.sha !== expectedHeadSha) return null
|
||||
return { ...event, pull_request: pull }
|
||||
}
|
||||
|
||||
@@ -366,21 +413,29 @@ export async function approvalEventFromComment({ event, api }) {
|
||||
return resolved
|
||||
}
|
||||
|
||||
function activeCommentDelegation(event, result) {
|
||||
if (!isRecord(event.issue) || !['created', 'edited'].includes(event.action) || !isRecord(event.comment)) return undefined
|
||||
return result.delegations.find(({ commentId }) => commentId === event.comment.id)
|
||||
/**
|
||||
* Refresh a pull-request event before any status write; closed PRs and stale heads are skipped.
|
||||
* @param {{event: unknown, api: (path: string) => Promise<unknown>}} options Pull-request event and API caller.
|
||||
* @returns {Promise<Record<string, unknown> | null>} Live event, or null for a closed PR or superseded head.
|
||||
*/
|
||||
export async function approvalEventFromPullRequest({ event, api }) {
|
||||
const expected = pullRequestFromEvent(event)
|
||||
const resolved = await approvalEventFromComment({ event: { ...event, issue: { number: expected.number, pull_request: {} } }, api })
|
||||
if (resolved === null || resolved.pull_request.head.sha !== expected.headSha) return null
|
||||
return { ...event, pull_request: resolved.pull_request }
|
||||
}
|
||||
|
||||
async function requestDelegatedReview(event, result, api, write) {
|
||||
const delegation = activeCommentDelegation(event, result)
|
||||
if (!delegation) return
|
||||
async function requestDelegatedReviews(result, api, write) {
|
||||
const recipients = new Map(result.delegations.filter(({ approved }) => !approved)
|
||||
.map(({ delegatedTo }) => [delegatedTo.toLowerCase(), delegatedTo]))
|
||||
if (!recipients.size) return
|
||||
const path = `/repos/${result.pull.repository}/pulls/${result.pull.number}/requested_reviewers`
|
||||
const requested = await api(path)
|
||||
if (!isRecord(requested) || !Array.isArray(requested.users)) throw new Error('requested reviewers response has no users array')
|
||||
const logins = requested.users.map(user => validateLogin(user?.login, 'requested reviewer').toLowerCase())
|
||||
if (logins.includes(delegation.delegatedTo.toLowerCase())) return
|
||||
await api(path, { method: 'POST', body: { reviewers: [delegation.delegatedTo] } })
|
||||
write(`Requested review from @${delegation.delegatedTo} for @${delegation.login}'s delegation.`)
|
||||
for (const user of requested.users) recipients.delete(validateLogin(user?.login, 'requested reviewer').toLowerCase())
|
||||
if (!recipients.size) return
|
||||
await api(path, { method: 'POST', body: { reviewers: [...recipients.values()] } })
|
||||
write(`Requested delegated reviews from ${[...recipients.values()].map(login => `@${login}`).join(', ')}.`)
|
||||
}
|
||||
|
||||
function approvalResult(pull, requiredPoints, approvals, blockers, ignoredReviewers, state, detail, authorCredit = null, delegations = []) {
|
||||
@@ -504,22 +559,19 @@ async function main() {
|
||||
})
|
||||
if (isRecord(event) && isRecord(event.issue)) {
|
||||
event = await approvalEventFromComment({ event, api })
|
||||
if (event === null) return
|
||||
} else if (isRecord(event) && isRecord(event.workflow_run)) {
|
||||
event = await approvalEventFromWorkflowRun({ event, api })
|
||||
} else {
|
||||
event = await approvalEventFromPullRequest({ event, api })
|
||||
}
|
||||
if (isRecord(event) && isRecord(event.workflow_run)) {
|
||||
const resolved = await approvalEventFromWorkflowRun({
|
||||
event,
|
||||
api,
|
||||
})
|
||||
if (resolved === null) {
|
||||
process.stdout.write('Skipped a review event for a superseded pull-request head.\n')
|
||||
return
|
||||
}
|
||||
event = resolved
|
||||
if (event === null) {
|
||||
process.stdout.write('Skipped an issue, closed pull request, or superseded pull-request head.\n')
|
||||
return
|
||||
}
|
||||
const phase = process.argv[2]
|
||||
if (phase) {
|
||||
await publishApprovalPhase({ event, api, runUrl: process.env.GITHUB_RUN_URL ?? '', phase })
|
||||
if (phase === 'pending' && process.env.GITHUB_OUTPUT) appendFileSync(process.env.GITHUB_OUTPUT, 'active=true\n')
|
||||
return
|
||||
}
|
||||
await runApprovalCheck({
|
||||
|
||||
@@ -5,6 +5,7 @@ import test from 'node:test'
|
||||
import {
|
||||
approvalEventFromWorkflowRun,
|
||||
approvalEventFromComment,
|
||||
approvalEventFromPullRequest,
|
||||
createGitHubApi,
|
||||
effectiveReviewDecisions,
|
||||
evaluateApproval as evaluateWithHistory,
|
||||
@@ -24,6 +25,7 @@ const pullRequestEvent = ({ author = 'author', draft = false } = {}) => ({
|
||||
repository: { full_name: 'deepseek-harness/deepseek-harness' },
|
||||
pull_request: {
|
||||
number: 42,
|
||||
state: 'open',
|
||||
draft,
|
||||
user: { login: author, node_id: 'author-id' },
|
||||
head: { sha: HEAD_SHA },
|
||||
@@ -623,10 +625,17 @@ test('bot authors receive the same history credit', async () => {
|
||||
assert.equal(result.state, 'success')
|
||||
})
|
||||
|
||||
const comment = (login, body, created_at = '2026-09-15T00:00:00Z', id = 1) => ({ user: { login }, body, created_at, id })
|
||||
const comment = (login, body, created_at = '2026-09-15T00:00:00Z', id = 1) => ({
|
||||
user: { login, node_id: `account:${login.toLowerCase()}` }, body, created_at, id, node_id: `comment:${login}:${id}:${body}:${created_at}`,
|
||||
})
|
||||
|
||||
function delegationApi({ comments = [], reviews = [], permissions = {} } = {}) {
|
||||
return async path => {
|
||||
return async (path, options) => {
|
||||
if (path === '/graphql') return { data: { nodes: options.body.variables.ids.map(id => {
|
||||
const entry = comments.find(comment => comment.node_id === id)
|
||||
return { id, body: entry.body, createdAt: entry.created_at, author: { id: entry.user.node_id }, lastEditedAt: null, editor: null,
|
||||
...entry.editorMetadata }
|
||||
}) } }
|
||||
if (path.includes('/comments?')) return comments
|
||||
if (path.includes('/reviews?')) return reviews
|
||||
const match = /\/collaborators\/([^/]+)\/permission$/u.exec(path)
|
||||
@@ -659,7 +668,7 @@ test('delegates the sender fixed points once after the recipient approves, retai
|
||||
...(senderState === 'CHANGES_REQUESTED' ? ['turtle1999'] : []),
|
||||
...(recipientState === 'CHANGES_REQUESTED' ? ['writer'] : []),
|
||||
])
|
||||
if (points) assert.deepEqual(result.approvals[0], { login: 'turtle1999', points: 2, delegatedTo: 'writer' })
|
||||
if (points) assert.deepEqual(result.approvals[0], { login: senderState ? 'turtle1999' : 'Turtle1999', points: 2, delegatedTo: 'writer' })
|
||||
}
|
||||
}
|
||||
})
|
||||
@@ -806,23 +815,20 @@ test('an active delegate command requests review once and preserves other reques
|
||||
return { users: [{ login: 'another-reviewer' }, ...(alreadyRequested ? [{ login: 'WRITER' }] : [])], teams: [] }
|
||||
}
|
||||
if (path.includes('/statuses/')) return {}
|
||||
return api(path)
|
||||
return api(path, options)
|
||||
},
|
||||
})
|
||||
assert.equal(result.points, 0)
|
||||
assert.deepEqual(requests, alreadyRequested ? [] : [{ reviewers: ['writer'] }])
|
||||
assert.equal(output.includes("Requested review from @writer for @turtle1999's delegation."), !alreadyRequested)
|
||||
assert.equal(output.includes("Requested delegated reviews from @writer."), !alreadyRequested)
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
test('inactive commands, drafts, and non-comment events do not request review', async () => {
|
||||
test('inactive commands and drafts do not request review', async () => {
|
||||
const command = comment('turtle1999', '/delegate @writer')
|
||||
for (const scenario of [
|
||||
{ event: {} },
|
||||
{ event: { issue: { number: 42, pull_request: {} }, action: 'deleted', comment: { id: 1 } } },
|
||||
{ comments: [comment('turtle1999', '/delegate @turtle1999')] },
|
||||
{ comments: [command, comment('turtle1999', '/delegate @other', '2026-09-16T00:00:00Z', 2)] },
|
||||
{ reviews: [review('turtle1999', 'COMMENTED', '2026-09-16T00:00:00Z')] },
|
||||
{ permissions: { turtle1999: 'read' } },
|
||||
{ permissions: { writer: 'read' } },
|
||||
@@ -843,23 +849,26 @@ test('inactive commands, drafts, and non-comment events do not request review',
|
||||
}
|
||||
})
|
||||
|
||||
test('failed review requests publish an error status', async () => {
|
||||
test('failed review requests preserve the computed score and report the retry', async () => {
|
||||
for (const failedRead of [false, true]) {
|
||||
const statuses = []
|
||||
const api = delegationApi({ comments: [comment('turtle1999', '/delegate @writer')] })
|
||||
await assert.rejects(runWithHistory({
|
||||
const api = delegationApi({ comments: [comment('turtle1999', '/delegate @writer')], reviews: [review('imccyu', 'APPROVED')] })
|
||||
const output = []
|
||||
await runWithHistory({
|
||||
event: { ...pullRequestEvent(), issue: { number: 42, pull_request: {} }, action: 'created', comment: { id: 1 } },
|
||||
policySource, runUrl: 'https://github.example/run/1', getMergedCount: async () => 0, write: () => {},
|
||||
policySource, runUrl: 'https://github.example/run/1', getMergedCount: async () => 0, getOwnership: async () => ({ totalLines: 0, reviewerLines: {} }), write: line => output.push(line),
|
||||
api: async (path, options) => {
|
||||
if (path.includes('/statuses/')) { statuses.push(options.body.state); return {} }
|
||||
if (path.endsWith('/requested_reviewers')) {
|
||||
if (options?.method === 'POST') throw new Error('request rejected')
|
||||
return failedRead ? {} : { users: [] }
|
||||
}
|
||||
return api(path)
|
||||
return api(path, options)
|
||||
},
|
||||
}), failedRead ? /no users array/u : /request rejected/u)
|
||||
assert.deepEqual(statuses, ['pending', 'error'])
|
||||
})
|
||||
assert.match(output.at(-1), failedRead ? /no users array/u : /request rejected/u)
|
||||
assert.match(output.at(-1), /score is unchanged.*will retry/u)
|
||||
assert.deepEqual(statuses, ['pending', 'success'])
|
||||
}
|
||||
})
|
||||
|
||||
@@ -884,12 +893,12 @@ test('delegation dismisses only the sender old decisions and stays active across
|
||||
if (options?.method === 'POST') calls.push(options)
|
||||
return { users: [] }
|
||||
}
|
||||
return read(path)
|
||||
return read(path, options)
|
||||
}
|
||||
const result = await runWithHistory({ event: { ...pullRequestEvent(), issue: { number: 42, pull_request: {} }, action: 'created', comment: { id: 1 } },
|
||||
policySource, api, runUrl: 'https://github.example/run/1', write: () => {}, getMergedCount: async () => 0,
|
||||
})
|
||||
assert.deepEqual(calls.map(call => call.method), ['PUT', 'PUT', 'POST'])
|
||||
assert.deepEqual(calls.map(call => call.method), ['PUT', 'PUT'])
|
||||
assert.equal(calls[0].body.event, 'DISMISS')
|
||||
assert.equal(calls[0].body.message, 'This is by automated Angry Turtle Cyborg, not a human. @turtle1999 delegated approval to @writer via /delegate.')
|
||||
assert.deepEqual(result.blockers, ['other'])
|
||||
@@ -943,7 +952,7 @@ test('a failed or unconfirmed dismissal prevents review requests and publishes e
|
||||
return { id: 10, state: 'CHANGES_REQUESTED' }
|
||||
}
|
||||
assert.ok(!path.endsWith('/requested_reviewers'))
|
||||
return read(path)
|
||||
return read(path, options)
|
||||
},
|
||||
}), rejected ? /dismissal denied/u : /dismissal was not confirmed/u)
|
||||
assert.deepEqual(states, ['pending', 'error'])
|
||||
@@ -953,21 +962,22 @@ test('a failed or unconfirmed dismissal prevents review requests and publishes e
|
||||
test('reads all comment pages, logs the score owner, and fails closed on missing comment history', async () => {
|
||||
const statuses = []
|
||||
const output = []
|
||||
const api = delegationApi({ reviews: [review('writer', 'APPROVED')] })
|
||||
const pageCommand = comment('turtle1999', '/delegate @writer')
|
||||
const api = delegationApi({ comments: [pageCommand], reviews: [review('writer', 'APPROVED')] })
|
||||
const run = comments => runWithHistory({
|
||||
event: pullRequestEvent(), policySource, runUrl: 'https://github.example/run/1',
|
||||
getMergedCount: async () => 0, write: line => output.push(line),
|
||||
api: async (path, options) => {
|
||||
if (path.includes('/statuses/')) { statuses.push(options.body.state); return {} }
|
||||
if (path.includes('/comments?')) return comments(path)
|
||||
return api(path)
|
||||
return api(path, options)
|
||||
},
|
||||
})
|
||||
const pages = []
|
||||
await run(path => {
|
||||
pages.push(path)
|
||||
return path.endsWith('page=1') ? Array.from({ length: 100 }, () => comment('writer', 'text'))
|
||||
: [comment('turtle1999', '/delegate @writer')]
|
||||
: [pageCommand]
|
||||
})
|
||||
assert.equal(pages.length, 2)
|
||||
assert.ok(output.includes('- @turtle1999: 2 (delegated to @writer)'))
|
||||
@@ -1006,3 +1016,116 @@ test('comment events resolve the live PR head and skip ordinary issues and close
|
||||
await assert.rejects(approvalEventFromComment({ event, api: async () => response }))
|
||||
}
|
||||
})
|
||||
|
||||
test('only the author can authorize a command edit, on every event reconstruction', async () => {
|
||||
for (const editor of [{ id: 'account:attacker' }, null, { id: 'account:turtle1999' }]) {
|
||||
const command = { ...comment('turtle1999', '/delegate @attacker'), editorMetadata: {
|
||||
lastEditedAt: '2026-09-15T01:00:00Z', editor,
|
||||
} }
|
||||
const result = await evaluateDelegation({ api: delegationApi({ comments: [command], reviews: [review('attacker', 'APPROVED')] }) })
|
||||
assert.equal(result.points, editor?.id === 'account:turtle1999' ? 3 : 1)
|
||||
assert.equal(result.delegations.length, editor?.id === 'account:turtle1999' ? 1 : 0)
|
||||
}
|
||||
})
|
||||
|
||||
test('ineligible or third-party edited commands do not replace an earlier eligible delegation', async () => {
|
||||
for (const later of [comment('turtle1999', '/delegate @reader'), comment('turtle1999', '/delegate @author'),
|
||||
{ ...comment('turtle1999', '/delegate @turtle1999'), editorMetadata: { lastEditedAt: '2026-09-15T01:00:00Z', editor: { id: 'account:attacker' } } }]) {
|
||||
const result = await evaluateDelegation({ api: delegationApi({
|
||||
comments: [comment('turtle1999', '/delegate @writer'), later],
|
||||
reviews: [review('writer', 'APPROVED')], permissions: { reader: 'read' },
|
||||
}) })
|
||||
assert.equal(result.points, 3)
|
||||
assert.equal(result.delegations[0].delegatedTo, 'writer')
|
||||
}
|
||||
})
|
||||
|
||||
test('missing, partial, changed, or failed editor metadata prevents scoring', async () => {
|
||||
const command = comment('turtle1999', '/delegate @writer')
|
||||
const read = delegationApi({ comments: [command], reviews: [review('writer', 'APPROVED')] })
|
||||
for (const response of [null, {}, { data: { nodes: [] } }, { data: { nodes: [null] } }, { errors: [{ message: 'forbidden' }] }]) {
|
||||
await assert.rejects(evaluateDelegation({ api: (path, options) => path === '/graphql' ? response : read(path, options) }), /editor history/u)
|
||||
}
|
||||
for (const patch of [{ body: '/delegate @attacker' }, { author: { id: 'account:attacker' } }, { lastEditedAt: undefined }, { id: 'other-comment' }]) {
|
||||
await assert.rejects(evaluateDelegation({ api: delegationApi({ comments: [{ ...command, editorMetadata: patch }] }) }), /editor history/u)
|
||||
}
|
||||
await assert.rejects(evaluateDelegation({ api: (path, options) => {
|
||||
if (path === '/graphql') throw new Error('editor API unavailable')
|
||||
return read(path, options)
|
||||
} }), /editor API unavailable/u)
|
||||
})
|
||||
|
||||
test('editor verification batches complete command history', async () => {
|
||||
const comments = Array.from({ length: 101 }, (_, index) => comment('turtle1999', '/delegate @writer', undefined, index + 1))
|
||||
const read = delegationApi({ comments })
|
||||
const batches = []
|
||||
const result = await evaluateDelegation({ api: (path, options) => {
|
||||
if (path.includes('/comments?')) return path.endsWith('page=1') ? comments.slice(0, 100) : comments.slice(100)
|
||||
if (path === '/graphql') batches.push(options.body.variables.ids.length)
|
||||
return read(path, options)
|
||||
} })
|
||||
assert.deepEqual(batches, [100, 1])
|
||||
assert.equal(result.delegations[0].commentId, 101)
|
||||
})
|
||||
|
||||
test('later events reconcile all active delegations after a draft or replaced comment event', async () => {
|
||||
for (const action of ['ready_for_review', 'synchronize', 'submitted', 'deleted']) {
|
||||
const reviews = [review('Turtle1999', 'CHANGES_REQUESTED', undefined, 10), review('second', 'APPROVED', undefined, 11)]
|
||||
const read = delegationApi({ comments: [comment('Turtle1999', '/delegate @Writer'), comment('second', '/delegate @WRITER')], reviews })
|
||||
const effects = []
|
||||
const requested = []
|
||||
const api = async (path, options) => {
|
||||
if (path.includes('/statuses/')) return {}
|
||||
if (path.endsWith('/dismissals')) {
|
||||
const old = reviews.find(review => path.endsWith(`/reviews/${review.id}/dismissals`))
|
||||
effects.push(old.id)
|
||||
old.state = 'DISMISSED'
|
||||
return old
|
||||
}
|
||||
if (path.endsWith('/requested_reviewers')) {
|
||||
if (options?.method === 'POST') { effects.push(options.body); requested.push({ login: 'writer' }) }
|
||||
return { users: requested }
|
||||
}
|
||||
return read(path, options)
|
||||
}
|
||||
const run = draft => runWithHistory({ event: { ...pullRequestEvent({ draft }), action }, policySource, api,
|
||||
runUrl: 'https://github.example/run/1', getMergedCount: async () => 0, write: () => {},
|
||||
})
|
||||
await run(true)
|
||||
assert.deepEqual(effects, [])
|
||||
const result = await run(false)
|
||||
assert.deepEqual(effects, [10, 11, { reviewers: ['Writer'] }])
|
||||
assert.equal(result.delegations[0].login, 'Turtle1999')
|
||||
await run(false)
|
||||
assert.equal(effects.length, 3)
|
||||
}
|
||||
})
|
||||
|
||||
test('already approved delegates need no new request', async () => {
|
||||
const read = delegationApi({ comments: [comment('turtle1999', '/delegate @writer')],
|
||||
reviews: [review('writer', 'APPROVED')],
|
||||
})
|
||||
const result = await runWithHistory({ event: pullRequestEvent(), policySource, runUrl: 'https://github.example/run/1', write: () => {},
|
||||
api: (path, options) => {
|
||||
if (path.includes('/statuses/')) return {}
|
||||
assert.ok(!path.endsWith('/requested_reviewers'))
|
||||
return read(path, options)
|
||||
},
|
||||
})
|
||||
assert.equal(result.state, 'success')
|
||||
})
|
||||
|
||||
test('pull-request events use live state and skip closed, merged, and superseded PRs', async () => {
|
||||
const event = pullRequestEvent()
|
||||
for (const patch of [{ state: 'closed' }, { state: 'closed', merged: true, merge_commit_sha: HEAD_SHA },
|
||||
{ head: { sha: 'abcdef1234567890abcdef1234567890abcdef12' } }]) {
|
||||
assert.equal(await approvalEventFromPullRequest({ event, api: async () => ({ ...event.pull_request, ...patch }) }), null)
|
||||
}
|
||||
const ready = await approvalEventFromPullRequest({ event: pullRequestEvent({ draft: true }), api: async () => event.pull_request })
|
||||
assert.equal(ready.pull_request.draft, false)
|
||||
assert.equal(await approvalEventFromWorkflowRun({ event: {
|
||||
repository: event.repository,
|
||||
workflow_run: { path: '.github/workflows/weighted-approval-review-event.yml', event: 'pull_request_review',
|
||||
conclusion: 'success', head_sha: HEAD_SHA, display_title: 'weighted-approval-review-event:42', pull_requests: [] },
|
||||
}, api: async () => ({ ...event.pull_request, state: 'closed', merged: true }) }), null)
|
||||
})
|
||||
@@ -9,6 +9,7 @@ permissions: {}
|
||||
|
||||
jobs:
|
||||
record-review-event:
|
||||
if: github.event.pull_request.state == 'open'
|
||||
name: record weighted approval review event
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 2
|
||||
|
||||
@@ -20,7 +20,11 @@ concurrency:
|
||||
|
||||
jobs:
|
||||
publish-status:
|
||||
if: (github.event_name != 'workflow_run' || github.event.workflow_run.conclusion == 'success') && (github.event_name != 'issue_comment' || github.event.issue.pull_request)
|
||||
if: >-
|
||||
(github.event_name != 'pull_request_target' || github.event.pull_request.state == 'open') &&
|
||||
(github.event_name != 'workflow_run' || github.event.workflow_run.conclusion == 'success') &&
|
||||
(github.event_name != 'issue_comment' || (github.event.issue.pull_request && github.event.issue.state == 'open' &&
|
||||
(contains(github.event.comment.body, '/delegate') || contains(github.event.changes.body.from, '/delegate'))))
|
||||
name: weighted approval publisher
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 5
|
||||
@@ -39,20 +43,23 @@ jobs:
|
||||
GITHUB_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
|
||||
run: node .github/review-ownership/check-approval.mjs pending
|
||||
# SECURITY: dependency setup runs in the status-writing job.
|
||||
- uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
|
||||
- if: steps.revoke.outputs.active == 'true'
|
||||
uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
|
||||
with:
|
||||
python-version: '3.10'
|
||||
cache: pip
|
||||
cache-dependency-path: .github/review-ownership/requirements.txt
|
||||
- name: Install production lexer
|
||||
if: steps.revoke.outputs.active == 'true'
|
||||
run: python3 -m pip install -r .github/review-ownership/requirements.txt
|
||||
- name: Publish weighted approval status
|
||||
if: steps.revoke.outputs.active == 'true'
|
||||
env:
|
||||
GITHUB_TOKEN: ${{ github.token }}
|
||||
GITHUB_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
|
||||
run: node .github/review-ownership/check-approval.mjs
|
||||
- name: Publish approval setup failure
|
||||
if: failure() && steps.revoke.outcome == 'success'
|
||||
if: failure() && steps.revoke.outputs.active == 'true'
|
||||
env:
|
||||
GITHUB_TOKEN: ${{ github.token }}
|
||||
GITHUB_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
|
||||
|
||||
Reference in new issue
Block a user