Raise author approval credit to 1.1 at 100 merged PRs
This commit is contained in:
1 parent
43e1e241ee
commit
c7e7e8bc18
7 files changed
+92
-21
No files matched your search
@@ -20,7 +20,7 @@ The weighted approval workflow exposes two pull-request checks. The `weighted ap
|
||||
|
||||
Reviewers whose calculated base repository permission is `write` or `admin` count. The [approval policy](approval-policy.json) gives `@07akioni`, `@imccyu`, `@tianyicui`, `@tianyicui-bot`, `@turtle1999`, and `@turtle2099` two points each; every other write-capable reviewer gets one point. The pull-request author’s own review and reviewers without write permission do not count.
|
||||
|
||||
The PR author contributes `min(0.6, mergedPRCount / 250)` points: 0 merged PRs → 0 points, 100 → 0.4, and 150 or more → 0.6. The publisher counts only merged PRs in this repository using the author’s immutable account ID, excluding the current PR. It stops at 150 matches and rejects incomplete history responses. Counts refresh at the next subscribed evaluation event. Author credit is separate from reviewer approvals and cannot satisfy the two-point requirement alone; the author’s own review remains excluded. Human and bot authors, including Dependabot, use the same rule. Drafts, blocking reviews, and sufficient reviewer points skip history lookup; logs mark credit as not evaluated. Otherwise, logs show author credit and the capped count separately. Below the cap, counting may scan the repository’s entire merged history; a failed required lookup publishes an error.
|
||||
The PR author contributes `min(100, mergedPRCount) × 11 / 1000` points: 0 merged PRs → 0 points, 50 → 0.55, and 100 or more → 1.1. The publisher counts only merged PRs in this repository using the author’s immutable account ID, excluding the current PR. It stops at 100 matches and rejects incomplete history responses. Counts refresh at the next subscribed evaluation event. Author credit is separate from reviewer approvals and cannot satisfy the two-point requirement alone; the author’s own review remains excluded. Human and bot authors, including Dependabot, use the same rule. Drafts, blocking reviews, and sufficient reviewer points skip history lookup; logs mark credit as not evaluated. Otherwise, logs show author credit and the capped count separately. Below the cap, counting may scan the repository’s entire merged history; a failed required lookup publishes an error.
|
||||
|
||||
A one-point approval receives weight `min(2, 1 + 4 × ownedLines / totalLines)` from modified or deleted old production-code lines, attributed by `git blame` at the merge base of the live base branch and exact reviewed head. Ownership of 0%, 12.5%, and 25% gives 1, 1.5, and 2 points; higher ownership remains capped at 2. The success threshold remains 2 total points, without rounding the score. New lines do not enter the denominator, and an empty denominator gives no boost. Existing two-point weights remain unchanged. GitHub commit-author accounts identify reviewers across author emails; unlinked authors remain in the denominator without contributing to a reviewer. The publisher logs measured ownership. It skips attribution when reviewer points plus author credit already meet the threshold or a blocking review exists. Displayed scores use at most three decimal places; the decision uses unrounded scores with a tolerance of `1e-12` points for floating-point error. The curve endpoints come from the policy’s default and required points.
|
||||
|
||||
|
||||
@@ -1,20 +1,20 @@
|
||||
/** Author credit from merged pull requests in the same repository. */
|
||||
const CAP_COUNT = 150
|
||||
const CAP_COUNT = 100
|
||||
|
||||
/**
|
||||
* Convert merged PR count to author points, capped at 0.6.
|
||||
* Convert merged PR count to author points, capped at 1.1.
|
||||
* @param {number} mergedCount Merged PR count.
|
||||
* @returns {number} Author approval points.
|
||||
*/
|
||||
export function authorCreditPoints(mergedCount) {
|
||||
return Math.min(CAP_COUNT, mergedCount) / 250
|
||||
return Math.min(CAP_COUNT, mergedCount) * 11 / 1000
|
||||
}
|
||||
|
||||
/**
|
||||
* Count merged PRs by immutable author account, stopping at the credit cap.
|
||||
* @param {{repository: string, number: number, authorId: string}} pull Current pull request.
|
||||
* @param {(path: string, options: object) => Promise<unknown>} api GitHub API caller.
|
||||
* @returns {Promise<number>} Merged count, capped at 150; incomplete responses reject.
|
||||
* @returns {Promise<number>} Merged count, capped at 100; incomplete responses reject.
|
||||
*/
|
||||
export async function countMergedAuthorPulls(pull, api) {
|
||||
const [owner, name] = pull.repository.split('/')
|
||||
|
||||
@@ -20,16 +20,16 @@ test('counts only the same account and excludes the current PR and deleted accou
|
||||
assert.equal(count, 1)
|
||||
})
|
||||
|
||||
test('follows cursors, deduplicates overlapping pages, and stops at 150', async () => {
|
||||
test('follows cursors, deduplicates overlapping pages, and stops at 100', async () => {
|
||||
let calls = 0
|
||||
const count = await countMergedAuthorPulls(pull, async (path, { body }) => {
|
||||
calls++
|
||||
if (calls === 1) return page(Array.from({ length: 100 }, (_, i) => entry(i + 1)), true, 'next')
|
||||
if (calls === 1) return page(Array.from({ length: 100 }, (_, i) => entry(i + 1, i < 50 ? pull.authorId : 'another-id')), true, 'next')
|
||||
assert.equal(calls, 2)
|
||||
assert.equal(body.variables.after, 'next')
|
||||
return page(Array.from({ length: 100 }, (_, i) => entry(i + 100)), true, 'unused')
|
||||
return page(Array.from({ length: 100 }, (_, i) => entry(i + 51)), true, 'unused')
|
||||
})
|
||||
assert.equal(count, 150)
|
||||
assert.equal(count, 100)
|
||||
assert.equal(calls, 2)
|
||||
})
|
||||
|
||||
|
||||
@@ -485,7 +485,7 @@ test('publishes setup phases without evaluating or installing dependencies', asy
|
||||
await assert.rejects(publishApprovalPhase({ ...options, phase: 'success' }), /invalid approval setup phase/u)
|
||||
})
|
||||
|
||||
for (const [mergedCount, credit] of [[0, 0], [1, 0.004], [50, 0.2], [100, 0.4], [125, 0.5], [150, 0.6], [200, 0.6]]) {
|
||||
for (const [mergedCount, credit] of [[0, 0], [1, 0.011], [50, 0.55], [99, 1.089], [100, 1.1], [101, 1.1], [200, 1.1]]) {
|
||||
test(`author with ${mergedCount} merged PRs contributes ${credit} points but cannot approve alone`, async () => {
|
||||
const result = await evaluateApproval({
|
||||
event: pullRequestEvent(), policySource, getMergedCount: async () => mergedCount,
|
||||
@@ -499,16 +499,31 @@ for (const [mergedCount, credit] of [[0, 0], [1, 0.004], [50, 0.2], [100, 0.4],
|
||||
}
|
||||
|
||||
test('combines author credit and reviewer ownership at the passing threshold', async () => {
|
||||
for (let mergedCount = 0; mergedCount <= 150; mergedCount++) {
|
||||
for (let mergedCount = 0; mergedCount <= 90; mergedCount++) {
|
||||
const result = await evaluateApproval({
|
||||
event: pullRequestEvent(), policySource, getMergedCount: async () => mergedCount,
|
||||
getOwnership: async () => ({ totalLines: 1000, reviewerLines: { writer: 250 - mergedCount } }),
|
||||
getOwnership: async () => ({ totalLines: 1000000, reviewerLines: { writer: 250000 - 2750 * mergedCount } }),
|
||||
api: async path => path.includes('/reviews?') ? [review('writer', 'APPROVED')] : { permission: 'write' },
|
||||
})
|
||||
assert.equal(result.state, 'success', `author merged ${mergedCount}`)
|
||||
}
|
||||
})
|
||||
|
||||
for (const [mergedCount, state] of [[90, 'pending'], [91, 'success'], [100, 'success']]) {
|
||||
test(`one ordinary approval with ${mergedCount} merged PRs is ${state}`, async () => {
|
||||
const result = await evaluateApproval({
|
||||
event: pullRequestEvent(), policySource, getMergedCount: async () => mergedCount,
|
||||
getOwnership: async () => {
|
||||
assert.equal(mergedCount, 90, 'sufficient author credit must skip ownership lookup')
|
||||
return { totalLines: 0, reviewerLines: {} }
|
||||
},
|
||||
api: async path => path.includes('/reviews?') ? [review('writer', 'APPROVED')] : { permission: 'write' },
|
||||
})
|
||||
assert.equal(result.state, state)
|
||||
assert.equal(result.approvals[0].points, 1)
|
||||
})
|
||||
}
|
||||
|
||||
test('blockers skip history and ignore non-write reviewers', async () => {
|
||||
const result = await evaluateApproval({
|
||||
event: pullRequestEvent(), policySource, getMergedCount: async () => { throw new Error('blockers must skip history') },
|
||||
@@ -544,8 +559,8 @@ test('drafts skip history and history failures revoke success with error', async
|
||||
|
||||
test('a score just below the threshold remains pending even if its display rounds to two', async () => {
|
||||
const result = await evaluateApproval({
|
||||
event: pullRequestEvent(), policySource, getMergedCount: async () => 150,
|
||||
getOwnership: async () => ({ totalLines: 1000000, reviewerLines: { writer: 99999 } }),
|
||||
event: pullRequestEvent(), policySource, getMergedCount: async () => 50,
|
||||
getOwnership: async () => ({ totalLines: 1000000, reviewerLines: { writer: 112499 } }),
|
||||
api: async path => path.includes('/reviews?') ? [review('writer', 'APPROVED')] : { permission: 'write' },
|
||||
})
|
||||
assert.equal(result.state, 'pending')
|
||||
@@ -558,13 +573,13 @@ test('the publisher counts merged history through the production API path', asyn
|
||||
const output = []
|
||||
const result = await runWithHistory({
|
||||
event, policySource, runUrl: 'https://github.example/run/1', write: line => output.push(line),
|
||||
getOwnership: async () => ({ totalLines: 8, reviewerLines: { writer: 1 } }),
|
||||
getOwnership: async () => ({ totalLines: 80, reviewerLines: { writer: 9 } }),
|
||||
api: async (path, options) => {
|
||||
if (path.includes('/comments?')) return []
|
||||
if (path === '/graphql') {
|
||||
assert.equal(options.body.variables.owner, 'deepseek-harness')
|
||||
return { data: { repository: { pullRequests: {
|
||||
nodes: Array.from({ length: options.body.variables.after ? 25 : 100 }, (_, i) => ({
|
||||
nodes: Array.from({ length: 25 }, (_, i) => ({
|
||||
number: i + (options.body.variables.after ? 200 : 100), author: { id: event.pull_request.user.node_id },
|
||||
})),
|
||||
pageInfo: { hasNextPage: !options.body.variables.after, endCursor: 'next' },
|
||||
@@ -577,10 +592,10 @@ test('the publisher counts merged history through the production API path', asyn
|
||||
},
|
||||
})
|
||||
assert.equal(result.points, 2)
|
||||
assert.equal(result.authorCredit.points, 0.5)
|
||||
assert.equal(result.approvals[0].points, 1.5)
|
||||
assert.equal(result.authorCredit.points, 0.55)
|
||||
assert.equal(result.approvals[0].points, 1.45)
|
||||
assert.deepEqual(states, ['pending', 'success'])
|
||||
assert.equal(output[0], 'Author credit: 0.5 (125 merged PRs).')
|
||||
assert.equal(output[0], 'Author credit: 0.55 (50 merged PRs).')
|
||||
})
|
||||
|
||||
test('sufficient reviewer points and drafts publish without querying author history', async () => {
|
||||
@@ -617,11 +632,11 @@ test('bot authors receive the same history credit', async () => {
|
||||
const event = pullRequestEvent({ author: 'dependabot[bot]' })
|
||||
event.pull_request.user.type = 'Bot'
|
||||
const result = await evaluateApproval({
|
||||
event, policySource, getMergedCount: async () => 150,
|
||||
event, policySource, getMergedCount: async () => 100,
|
||||
getOwnership: async () => ({ totalLines: 10, reviewerLines: { writer: 1 } }),
|
||||
api: async path => path.includes('/reviews?') ? [review('writer', 'APPROVED')] : { permission: 'write' },
|
||||
})
|
||||
assert.equal(result.authorCredit.points, 0.6)
|
||||
assert.equal(result.authorCredit.points, 1.1)
|
||||
assert.equal(result.state, 'success')
|
||||
})
|
||||
|
||||
|
||||
Reference in new issue
Block a user