fix: reconcile automated review requests

2026-09-13 04:03:30 +00:00 · 2026-09-08 16:05:09 +08:00
parent f96fbba2db
commit 614ff04842
4 changed files with 166 additions and 24 deletions
@@ -18,9 +18,11 @@ 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 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.
+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 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.
+On every run with current review requests, the workflow reads the pull-request timeline. A current reviewer is workflow-authored only when the latest matching `review_requested` event names `github-actions[bot]` as `review_requester`; a request without an attributable event is preserved. On a non-draft pull request, the workflow cancels workflow-authored reviewers that no longer match the current candidates and excess workflow-authored non-turtle reviewers above the counted limit. Current relevance order decides which matching workflow reviewer remains when the limit shrinks. It then fills any slot left by the planned cancellations. On a draft, it cancels every current workflow-authored request. Requests made by people remain unchanged in both states. An attributable event with invalid provenance fails before mutation, and the workflow also fails without cancellation when the timeline exceeds 3,000 events.
+
+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 after planned cancellations; 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 remain 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.

@@ -48,7 +50,7 @@ Ownership changes take effect only after they merge into the default branch. Thi

 ## Verification

-Run `pnpm run test:request-review` for ownership parsing, file classification, complete-patch checks, comment parsing, changed-LOC ranking, pagination, logging order, draft cancellation, reviewer filtering, and API behavior. [Workflow tests](../../scripts/ci-workflow.spec.ts) pin the trusted checkout, permissions, events, and command. The repository gate graph runs both checks in CI.
+Run `pnpm run test:request-review` for ownership parsing, file classification, complete-patch checks, comment parsing, changed-LOC ranking, pagination, logging order, non-draft reconciliation, draft cancellation, reviewer provenance, reviewer filtering, and API behavior. [Workflow tests](../../scripts/ci-workflow.spec.ts) pin the trusted checkout, permissions, events, and command. The repository gate graph runs both checks in CI.

 <a id="dev-note"></a>

@@ -437,8 +437,8 @@ function requestedReviewerLogins(response) {
 }

 /**
- * Print changed paths, request missing owners on reviewable pull requests, and
- * cancel workflow-authored requests on drafts.
+ * Print changed paths, reconcile workflow-authored requests with current
+ * ownership, and cancel workflow-authored requests on drafts.
 * @param {{event: unknown, ownershipSource: string, api: (path: string, options?: {method?: string, body?: unknown}) => Promise<unknown>, write?: (line: string) => void}} options Runtime inputs.
 * @returns {Promise<{changedCodeFiles: string[], excludedTestFiles: string[], excludedDocumentationFiles: string[], excludedCommentOnlyFiles: string[], requestedReviewers: string[], cancelledReviewers: string[]}>} Applied routing result.
 */
@@ -494,17 +494,41 @@ export async function requestReviews({ event, ownershipSource, api, write = line
    return { ...classified, requestedReviewers: [], cancelledReviewers: reviewers }
  }

-  if (candidates.length === 0) {
-    writeList(write, 'Reviewers to request', [])
-    return { ...classified, requestedReviewers: [], cancelledReviewers: [] }
-  }
  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 workflowReviewers = currentReviewers.length === 0
+    ? []
+    : workflowRequestedReviewers(
+        await listPullRequestTimeline(api, pull.repository, pull.number),
+        currentReviewers,
+      )
+  const workflowReviewerKeys = new Set(workflowReviewers.map(login => login.toLowerCase()))
+  const manualReviewers = currentReviewers.filter(login => !workflowReviewerKeys.has(login.toLowerCase()))
+  let retainedCountedSlots = Math.max(
+    0,
+    MAX_COUNTED_REQUESTED_REVIEWERS
+      - manualReviewers.filter(login => login.toLowerCase() !== UNCOUNTED_REVIEWER).length,
+  )
+  const retainedWorkflowReviewerKeys = new Set()
+  for (const { login } of candidates) {
+    const key = login.toLowerCase()
+    if (!workflowReviewerKeys.has(key)) continue
+    if (key === UNCOUNTED_REVIEWER) retainedWorkflowReviewerKeys.add(key)
+    else if (retainedCountedSlots > 0) {
+      retainedWorkflowReviewerKeys.add(key)
+      retainedCountedSlots--
+    }
+  }
+  const reviewersToCancel = workflowReviewers.filter(
+    login => !retainedWorkflowReviewerKeys.has(login.toLowerCase()),
+  )
+  const cancelledReviewerKeys = new Set(reviewersToCancel.map(login => login.toLowerCase()))
+  const remainingReviewers = currentReviewers.filter(login => !cancelledReviewerKeys.has(login.toLowerCase()))
+  const alreadyRequested = new Set(remainingReviewers.map(login => login.toLowerCase()))
  const availableSlots = Math.max(
    0,
    MAX_COUNTED_REQUESTED_REVIEWERS
-      - currentReviewers.filter(login => login.toLowerCase() !== UNCOUNTED_REVIEWER).length,
+      - remainingReviewers.filter(login => login.toLowerCase() !== UNCOUNTED_REVIEWER).length,
  )
  writeList(write, 'Current individual review requests', currentReviewers.map(login => `@${login}`))
  write(`Available counted review request slots: ${availableSlots}.`)
@@ -512,15 +536,25 @@ export async function requestReviews({ event, ownershipSource, api, write = line
    .filter(({ login }) => !alreadyRequested.has(login.toLowerCase()))
    .slice(0, availableSlots)
    .map(({ login }) => login)
+  writeList(write, 'Review requests to cancel', reviewersToCancel.map(login => `@${login}`))
  writeList(write, 'Reviewers to request', reviewers.map(login => `@${login}`))
-  if (reviewers.length === 0) return { ...classified, requestedReviewers: [], cancelledReviewers: [] }
+  if (reviewersToCancel.length > 0) {
+    await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`, {
+      method: 'DELETE',
+      body: { reviewers: reviewersToCancel },
+    })
+    const requestLabel = reviewersToCancel.length === 1 ? 'request' : 'requests'
+    write(`Cancelled review ${requestLabel} for ${reviewersToCancel.map(login => `@${login}`).join(' ')}.`)
+  }

-  await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`, {
-    method: 'POST',
-    body: { reviewers },
-  })
-  write(`Requested ${reviewers.map(login => `@${login}`).join(' ')}.`)
-  return { ...classified, requestedReviewers: reviewers, cancelledReviewers: [] }
+  if (reviewers.length > 0) {
+    await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`, {
+      method: 'POST',
+      body: { reviewers },
+    })
+    write(`Requested ${reviewers.map(login => `@${login}`).join(' ')}.`)
+  }
+  return { ...classified, requestedReviewers: reviewers, cancelledReviewers: reviewersToCancel }
 }

 function pullRequestFromEvent(event) {
@@ -423,6 +423,7 @@ test('does not add another counted owner when one is already requested', async (
      if (path.endsWith('/requested_reviewers') && options.method === undefined) {
        return { users: [{ login: 'first' }], teams: [] }
      }
+      if (path.endsWith('/timeline?per_page=100&page=1')) return []
      throw new Error(`unexpected API path ${path}`)
    },
    write: line => output.push(line),
@@ -430,10 +431,12 @@ test('does not add another counted owner when one is already requested', async (

  assert.deepEqual(result.requestedReviewers, [])
  assert.equal(calls.some(call => call.options.method === 'POST'), false)
-  assert.deepEqual(output.slice(-5), [
+  assert.deepEqual(output.slice(-7), [
    'Current individual review requests:',
    '- @first',
    'Available counted review request slots: 0.',
+    'Review requests to cancel:',
+    '- (none)',
    'Reviewers to request:',
    '- (none)',
  ])
@@ -481,6 +484,7 @@ test('does not add turtle when one counted reviewer is already requested', async
      if (path.endsWith('/requested_reviewers') && options.method === undefined) {
        return { users: [{ login: 'first' }], teams: [] }
      }
+      if (path.endsWith('/timeline?per_page=100&page=1')) return []
      throw new Error(`unexpected API path ${path}`)
    },
    write: () => {},
@@ -503,6 +507,7 @@ test('keeps the counted slot available when turtle is already requested', async
      if (path.endsWith('/requested_reviewers') && options.method === undefined) {
        return { users: [{ login: 'turtle1999' }], teams: [] }
      }
+      if (path.endsWith('/timeline?per_page=100&page=1')) return []
      if (path.endsWith('/requested_reviewers') && options.method === 'POST') return {}
      throw new Error(`unexpected API path ${path}`)
    },
@@ -516,6 +521,101 @@ test('keeps the counted slot available when turtle is already requested', async
  })
 })

+test('replaces a workflow reviewer that no longer matches current ownership', async () => {
+  const trace = []
+  const result = await requestReviews({
+    event: pullRequestEvent({ author: 'contributor' }),
+    ownershipSource: '/packages/core/ @mektpoy\n',
+    api: async (path, options = {}) => {
+      trace.push({ type: 'api', 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: 'Dudu-0223' }], teams: [] }
+      }
+      if (path.endsWith('/timeline?per_page=100&page=1')) {
+        return [{
+          event: 'review_requested',
+          requested_reviewer: { login: 'Dudu-0223' },
+          review_requester: { login: 'github-actions[bot]' },
+        }]
+      }
+      if (path.endsWith('/requested_reviewers') && options.method === 'DELETE') return {}
+      if (path.endsWith('/requested_reviewers') && options.method === 'POST') return {}
+      throw new Error(`unexpected API path ${path}`)
+    },
+    write: line => trace.push({ type: 'log', line }),
+  })
+
+  assert.deepEqual(result.requestedReviewers, ['mektpoy'])
+  assert.deepEqual(result.cancelledReviewers, ['Dudu-0223'])
+  const cancelLog = trace.findIndex(item => item.type === 'log' && item.line === 'Review requests to cancel:')
+  const requestLog = trace.findIndex(item => item.type === 'log' && item.line === 'Reviewers to request:')
+  const firstMutation = trace.findIndex(item => item.type === 'api' && item.options.method !== undefined)
+  assert.ok(cancelLog >= 0 && requestLog >= 0 && cancelLog < firstMutation && requestLog < firstMutation)
+  assert.equal(trace[cancelLog + 1].line, '- @Dudu-0223')
+  assert.equal(trace[requestLog + 1].line, '- @mektpoy')
+  assert.deepEqual(trace.filter(item => item.type === 'api' && item.options.method !== undefined), [
+    {
+      type: 'api',
+      path: '/repos/deepseek-harness/deepseek-harness/pulls/42/requested_reviewers',
+      options: { method: 'DELETE', body: { reviewers: ['Dudu-0223'] } },
+    },
+    {
+      type: 'api',
+      path: '/repos/deepseek-harness/deepseek-harness/pulls/42/requested_reviewers',
+      options: { method: 'POST', body: { reviewers: ['mektpoy'] } },
+    },
+  ])
+})
+
+test('removes excess workflow reviewers using current relevance order', async () => {
+  const calls = []
+  const result = await requestReviews({
+    event: pullRequestEvent({ author: 'contributor', changedFiles: 2 }),
+    ownershipSource: '/packages/core/ @mektpoy\n/packages/subagent/ @Dudu-0223\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/subagent/subagent/src/index.ts', additions: 8, deletions: 2 },
+        ]
+      }
+      if (path.endsWith('/requested_reviewers') && options.method === undefined) {
+        return { users: [{ login: 'Dudu-0223' }, { login: 'mektpoy' }], teams: [] }
+      }
+      if (path.endsWith('/timeline?per_page=100&page=1')) {
+        return ['Dudu-0223', 'mektpoy'].map(login => ({
+          event: 'review_requested',
+          requested_reviewer: { login },
+          review_requester: { login: 'github-actions[bot]' },
+        }))
+      }
+      if (path.endsWith('/requested_reviewers') && options.method === 'DELETE') return {}
+      throw new Error(`unexpected API path ${path}`)
+    },
+    write: () => {},
+  })
+
+  assert.deepEqual(result, {
+    changedCodeFiles: [
+      'packages/core/agent/src/index.ts',
+      'packages/subagent/subagent/src/index.ts',
+    ],
+    excludedTestFiles: [],
+    excludedDocumentationFiles: [],
+    excludedCommentOnlyFiles: [],
+    requestedReviewers: [],
+    cancelledReviewers: ['Dudu-0223'],
+  })
+  assert.deepEqual(calls.find(call => call.options.method === 'DELETE'), {
+    path: '/repos/deepseek-harness/deepseek-harness/pulls/42/requested_reviewers',
+    options: { method: 'DELETE', body: { reviewers: ['Dudu-0223'] } },
+  })
+})
+
 test('does not request reviewers for test, documentation, or comment-only changes', async () => {
  const calls = []
  const output = []
@@ -535,7 +635,9 @@ test('does not request reviewers for test, documentation, or comment-only change
    ownershipSource,
    api: async (path) => {
      calls.push(path)
-      return files
+      if (path.endsWith('/files?per_page=100&page=1')) return files
+      if (path.endsWith('/requested_reviewers')) return { users: [], teams: [] }
+      throw new Error(`unexpected API path ${path}`)
    },
    write: line => output.push(line),
  })
@@ -547,7 +649,7 @@ test('does not request reviewers for test, documentation, or comment-only change
    requestedReviewers: [],
    cancelledReviewers: [],
  })
-  assert.equal(calls.length, 1)
+  assert.equal(calls.length, 2)
  assert.deepEqual(output.slice(0, 4), [
    'This is by automated Angry Turtle Cyborg, not a human',
    'Changed code files:',