fix: cap counted review requests at one

This commit is contained in:
Turtle
2026-09-08 15:50:36 +08:00
parent a04bffb7ed
commit f96fbba2db
5 changed files with 98 additions and 17 deletions
-1
View File
@@ -6,7 +6,6 @@
/native/ @mektpoy
/patches/ @mektpoy
/python/ @LegGasai
/scripts/ @turtle1999
/vendor/ @turtle1999
/website/ @LegGasai
/packages/acp/ @mektpoy
+2 -2
View File
@@ -18,9 +18,9 @@ The [`request-review` workflow](../workflows/request-review.yml) reads the CODEO
Pull requests run the workflow when opened, synchronized, reopened, marked ready for review, or converted to a draft. The scanner fetches the complete pull-request file list, evaluates both paths of a rename, and fails instead of routing from a partial list. GitHub exposes at most 3,000 files for this API.
For a non-draft pull request, the workflow requests missing matched owners while keeping the total number of current individual review requests at two or fewer. Existing individual requests consume those slots, including requests made by people outside the ownership map. When more candidates remain than available slots, the workflow ranks them by the total GitHub-reported additions plus deletions in reviewable changed-file records that match each owner. A rename contributes its changed LOC once to an owner even when both paths match that owner. Higher changed LOC ranks first, and login order resolves ties. The workflow does not remove requests from a non-draft pull request. For a draft, it reads the current requested reviewers and review-request timeline, then cancels each current request whose latest requester is `github-actions[bot]`. Current requests made by people remain unchanged. The workflow fails without cancellation when the timeline exceeds 3,000 events or contains invalid request provenance.
For a non-draft pull request, the workflow keeps at most one current individual review request other than `@turtle1999`; an existing request for `@turtle1999` does not consume that slot. Each run adds at most one reviewer. An existing non-turtle request leaves no slot, so the workflow does not add anyone, including `@turtle1999`. Existing individual requests consume the slot even when made by people outside the ownership map. When more candidates remain than the available counted slot can cover, the workflow ranks them by the total GitHub-reported additions plus deletions in reviewable changed-file records that match each owner. A rename contributes its changed LOC once to an owner even when both paths match that owner. Higher changed LOC ranks first, and login order resolves ties. The workflow does not remove requests from a non-draft pull request. For a draft, it reads the current requested reviewers and review-request timeline, then cancels each current request whose latest requester is `github-actions[bot]`. Current requests made by people remain unchanged. The workflow fails without cancellation when the timeline exceeds 3,000 events or contains invalid request provenance.
The ownership map accepts explicit absolute directory patterns and one or two individual GitHub users per pattern. It rejects wildcards, hidden-directory patterns, teams, more than two owners, and duplicate patterns or owners. Matching follows CODEOWNERS last-match semantics. The scanner prints the changed code, excluded test, documentation, and comment-only files; per-file owner matches and LOC; the aggregate owner relevance ranking; current individual requests and available slots; and the reviewers it will request or cancel before it mutates review requests. Unmatched files remain visible in the log. The pull-request author and users who are already requested are omitted from new requests.
The ownership map accepts explicit absolute directory patterns and one or two individual GitHub users per pattern. It rejects wildcards, hidden-directory patterns, teams, more than two owners, and duplicate patterns or owners. Matching follows CODEOWNERS last-match semantics. The scanner prints the changed code, excluded test, documentation, and comment-only files; per-file owner matches and LOC; the aggregate owner relevance ranking; current individual requests and the available counted slot; and the reviewers it will request or cancel before it mutates review requests. Unmatched files remain visible in the log. The pull-request author and users who are already requested are omitted from new requests.
The policy test measures non-test tracked lines under matched directories and requires `@turtle1999` to own no more than one third of that eligible owned codebase.
+8 -3
View File
@@ -7,9 +7,10 @@ import { pathToFileURL } from 'node:url'
const API_VERSION = '2026-03-10'
const MAX_OWNERS_PER_RULE = 2
const MAX_PULL_REQUEST_FILES = 3_000
const MAX_REQUESTED_REVIEWERS = 2
const MAX_COUNTED_REQUESTED_REVIEWERS = 1
const MAX_TIMELINE_EVENTS = 3_000
const PAGE_SIZE = 100
const UNCOUNTED_REVIEWER = 'turtle1999'
const WORKFLOW_REVIEW_REQUESTER = 'github-actions[bot]'
const TEST_DIRECTORY_NAMES = new Set(['__snapshots__', '__tests__', 'benches', 'stress-tests', 'test', 'tests'])
const TEST_FILE_MARKER = /\.(?:bench|corpus|e2e|perf|snapshot|spec|stress|test)\.[^./]+$/u
@@ -500,9 +501,13 @@ export async function requestReviews({ event, ownershipSource, api, write = line
const existing = await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`)
const currentReviewers = requestedReviewerLogins(existing).sort((left, right) => left.localeCompare(right, 'en'))
const alreadyRequested = new Set(currentReviewers.map(login => login.toLowerCase()))
const availableSlots = Math.max(0, MAX_REQUESTED_REVIEWERS - alreadyRequested.size)
const availableSlots = Math.max(
0,
MAX_COUNTED_REQUESTED_REVIEWERS
- currentReviewers.filter(login => login.toLowerCase() !== UNCOUNTED_REVIEWER).length,
)
writeList(write, 'Current individual review requests', currentReviewers.map(login => `@${login}`))
write(`Available review request slots: ${availableSlots}.`)
write(`Available counted review request slots: ${availableSlots}.`)
const reviewers = candidates
.filter(({ login }) => !alreadyRequested.has(login.toLowerCase()))
.slice(0, availableSlots)
@@ -32,8 +32,9 @@ const pullRequestEvent = ({ author = 'author', changedFiles = 1, draft = false }
test('loads the repository ownership policy without test-only directory rules', () => {
const rules = parseOwnership(ownershipSource)
const ownersByPattern = new Map(rules.map(rule => [rule.pattern, rule.owners]))
assert.equal(rules.length, 58)
assert.equal(rules.length, 57)
assert.equal(rules.some(rule => rule.pattern === '/benchmarks/'), false)
assert.equal(rules.some(rule => rule.pattern === '/scripts/'), false)
assert.equal(rules.some(rule => rule.pattern === '/snapshots/'), false)
assert.equal(rules.some(rule => rule.pattern === '/packages/test-support/'), false)
assert.deepEqual(ownersByPattern.get('/apps/cli/'), ['@turtle1999'])
@@ -344,7 +345,7 @@ test('fails closed when the review-request timeline exceeds its limit', async ()
assert.equal(calls, 30)
})
test('prints changed code files and limits current review requests to two people', async () => {
test('prints changed code files and requests the highest-ranked counted owner', async () => {
const trace = []
const files = [
{ filename: 'packages/core/agent/src/index.ts', additions: 70, deletions: 10 },
@@ -358,7 +359,7 @@ test('prints changed code files and limits current review requests to two people
trace.push({ type: 'api', path, options })
if (path.endsWith('/files?per_page=100&page=1')) return files
if (path.endsWith('/requested_reviewers') && options.method !== 'POST') {
return { users: [{ login: 'imccyu' }], teams: [] }
return { users: [], teams: [] }
}
if (path.endsWith('/requested_reviewers') && options.method === 'POST') return {}
throw new Error(`unexpected API path ${path}`)
@@ -408,19 +409,19 @@ test('prints changed code files and limits current review requests to two people
})
})
test('does not add an owner when two people are already requested', async () => {
test('does not add another counted owner when one is already requested', async () => {
const calls = []
const output = []
const result = await requestReviews({
event: pullRequestEvent(),
ownershipSource: '/packages/core/ @turtle1999 @mektpoy\n',
ownershipSource: '/packages/core/ @mektpoy\n',
api: async (path, options = {}) => {
calls.push({ path, options })
if (path.endsWith('/files?per_page=100&page=1')) {
return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
}
if (path.endsWith('/requested_reviewers') && options.method === undefined) {
return { users: [{ login: 'first' }, { login: 'second' }], teams: [] }
return { users: [{ login: 'first' }], teams: [] }
}
throw new Error(`unexpected API path ${path}`)
},
@@ -429,16 +430,92 @@ test('does not add an owner when two people are already requested', async () =>
assert.deepEqual(result.requestedReviewers, [])
assert.equal(calls.some(call => call.options.method === 'POST'), false)
assert.deepEqual(output.slice(-6), [
assert.deepEqual(output.slice(-5), [
'Current individual review requests:',
'- @first',
'- @second',
'Available review request slots: 0.',
'Available counted review request slots: 0.',
'Reviewers to request:',
'- (none)',
])
})
test('requests at most one owner per run when turtle ranks first', async () => {
const calls = []
const result = await requestReviews({
event: pullRequestEvent({ author: 'contributor', changedFiles: 2 }),
ownershipSource: '/packages/core/ @turtle1999\n/packages/client/ @mektpoy\n',
api: async (path, options = {}) => {
calls.push({ path, options })
if (path.endsWith('/files?per_page=100&page=1')) {
return [
{ filename: 'packages/core/agent/src/index.ts', additions: 25, deletions: 5 },
{ filename: 'packages/client/store/src/index.ts', additions: 8, deletions: 2 },
]
}
if (path.endsWith('/requested_reviewers') && options.method === undefined) {
return { users: [], teams: [] }
}
if (path.endsWith('/requested_reviewers') && options.method === 'POST') return {}
throw new Error(`unexpected API path ${path}`)
},
write: () => {},
})
assert.deepEqual(result.requestedReviewers, ['turtle1999'])
assert.deepEqual(calls.find(call => call.options.method === 'POST'), {
path: '/repos/deepseek-harness/deepseek-harness/pulls/42/requested_reviewers',
options: { method: 'POST', body: { reviewers: ['turtle1999'] } },
})
})
test('does not add turtle when one counted reviewer is already requested', async () => {
const calls = []
const result = await requestReviews({
event: pullRequestEvent({ author: 'contributor' }),
ownershipSource: '/packages/core/ @turtle1999 @mektpoy\n',
api: async (path, options = {}) => {
calls.push({ path, options })
if (path.endsWith('/files?per_page=100&page=1')) {
return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
}
if (path.endsWith('/requested_reviewers') && options.method === undefined) {
return { users: [{ login: 'first' }], teams: [] }
}
throw new Error(`unexpected API path ${path}`)
},
write: () => {},
})
assert.deepEqual(result.requestedReviewers, [])
assert.equal(calls.some(call => call.options.method === 'POST'), false)
})
test('keeps the counted slot available when turtle is already requested', async () => {
const calls = []
const result = await requestReviews({
event: pullRequestEvent({ author: 'contributor' }),
ownershipSource: '/packages/core/ @turtle1999 @mektpoy\n',
api: async (path, options = {}) => {
calls.push({ path, options })
if (path.endsWith('/files?per_page=100&page=1')) {
return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
}
if (path.endsWith('/requested_reviewers') && options.method === undefined) {
return { users: [{ login: 'turtle1999' }], teams: [] }
}
if (path.endsWith('/requested_reviewers') && options.method === 'POST') return {}
throw new Error(`unexpected API path ${path}`)
},
write: () => {},
})
assert.deepEqual(result.requestedReviewers, ['mektpoy'])
assert.deepEqual(calls.find(call => call.options.method === 'POST'), {
path: '/repos/deepseek-harness/deepseek-harness/pulls/42/requested_reviewers',
options: { method: 'POST', body: { reviewers: ['mektpoy'] } },
})
})
test('does not request reviewers for test, documentation, or comment-only changes', async () => {
const calls = []
const output = []