Procházet zdrojové kódy

Merge pull request #3788 from deepseek-harness/turtle/fix-approved-review-rerequest

fix(ci): do not re-request approved reviewers
Turtle před 1 měsícem
rodič
revize
b4983e92df

+ 8 - 4
.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.md

@@ -20,13 +20,15 @@ The scanner fetches every changed-file record before deciding. It fails if the p
 
 
 The scanner excludes test-only paths before owner matching. Excluded paths comprise directories named `test`, `tests`, `__tests__`, `__snapshots__`, `benches`, or `stress-tests`; the top-level `benchmarks` and `snapshots` trees; `packages/test-support`; `scripts/fixtures` and `scripts/snapshots`; filenames ending in `.bench.<ext>`, `.corpus.<ext>`, `.e2e.<ext>`, `.perf.<ext>`, `.snapshot.<ext>`, `.spec.<ext>`, `.stress.<ext>`, or `.test.<ext>`; and Python `test_*.py`, `*_test.py`, or `*_tests.py` files. Test infrastructure such as `vitest*.config.ts` and gate implementations remains reviewable because it changes how repository evidence is produced. The [comment-only routing decision](2026-09-08-comment-only-review-routing.md) owns the additional documentation and comment exclusions.
 The scanner excludes test-only paths before owner matching. Excluded paths comprise directories named `test`, `tests`, `__tests__`, `__snapshots__`, `benches`, or `stress-tests`; the top-level `benchmarks` and `snapshots` trees; `packages/test-support`; `scripts/fixtures` and `scripts/snapshots`; filenames ending in `.bench.<ext>`, `.corpus.<ext>`, `.e2e.<ext>`, `.perf.<ext>`, `.snapshot.<ext>`, `.spec.<ext>`, `.stress.<ext>`, or `.test.<ext>`; and Python `test_*.py`, `*_test.py`, or `*_tests.py` files. Test infrastructure such as `vitest*.config.ts` and gate implementations remains reviewable because it changes how repository evidence is produced. The [comment-only routing decision](2026-09-08-comment-only-review-routing.md) owns the additional documentation and comment exclusions.
 
 
-The workflow prints the changed code paths, each exclusion class, per-file owner matches and changed LOC, aggregate owner relevance, current individual requests, the available counted slot after planned cancellations, and final reviewer actions before any review-request mutation. For a non-draft pull request, it removes the pull-request author and users who remain requested from the matched individual owners, then keeps at most one current individual review request other than `@turtle1999`. An existing request for `@turtle1999` does not consume that slot, but each workflow 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 they do not match the ownership map. An owner's relevance is the sum of GitHub-reported additions and deletions for each reviewable changed-file record whose current or previous path matches that owner. Each record contributes once per owner, including when both paths of a rename match the same owner. Higher changed LOC selects candidates first when the available slot cannot cover the remaining owners; login order resolves equal scores.
+The workflow prints the changed code paths, each exclusion class, per-file owner matches and changed LOC, aggregate owner relevance, approved owners omitted from new requests, current individual requests, the available counted slot after planned cancellations, and final reviewer actions before any review-request mutation. For a non-draft pull request, it fetches the complete chronological review list and reduces each owner's undismissed `APPROVED` and `CHANGES_REQUESTED` reviews to the latest decisive state; `COMMENTED` and `PENDING` reviews leave that state unchanged. It removes the pull-request author, owners with an active approval, and users who remain requested from the matched individual owners. An active approval remains sufficient after later synchronize events, while a later changes-requested review makes the owner eligible again. The review-list operation fails before mutation at 3,000 entries or on an invalid record.
+
+The workflow keeps at most one current individual review request other than `@turtle1999`. An existing request for `@turtle1999` does not consume that slot, but each workflow 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 they do not match the ownership map. An owner's relevance is the sum of GitHub-reported additions and deletions for each reviewable changed-file record whose current or previous path matches that owner. Each record contributes once per owner, including when both paths of a rename match the same owner. Higher changed LOC selects candidates first when the available slot cannot cover the remaining owners; login order resolves equal scores.
 
 
 When current review requests exist, the workflow reads the complete review-request timeline before mutation. A current reviewer is workflow-authored only when its latest matching `review_requested` event identifies `github-actions[bot]` as `review_requester`; a request without an attributable event is preserved. A non-draft run 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 selects which matching workflow reviewer remains. Planned cancellations release capacity before the workflow selects a new reviewer. A draft run cancels every current workflow-authored request. Requests made by people remain unchanged. An attributable event with invalid provenance and timelines above 3,000 events fail before mutation.
 When current review requests exist, the workflow reads the complete review-request timeline before mutation. A current reviewer is workflow-authored only when its latest matching `review_requested` event identifies `github-actions[bot]` as `review_requester`; a request without an attributable event is preserved. A non-draft run 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 selects which matching workflow reviewer remains. Planned cancellations release capacity before the workflow selects a new reviewer. A draft run cancels every current workflow-authored request. Requests made by people remain unchanged. An attributable event with invalid provenance and timelines above 3,000 events fail before mutation.
 
 
 ## Verification
 ## Verification
 
 
-[Scanner tests](../../../../.github/review-ownership/request-review.test.mjs) cover admitted ownership syntax, rejected syntax, each exclusion class, production-name negative controls, renames, last-match behavior, unmatched files, changed-LOC aggregation and ranking, complete pagination, the 3,000-file limits, log-before-mutation ordering, author and existing-reviewer filtering, non-draft reconciliation, draft cancellation provenance, and API failures. [Workflow tests](../../../../scripts/ci-workflow.spec.ts) pin the event set, least permissions, trusted default-branch checkout, absence of pull-request-head references and secrets, and executed command. The gate graph includes both suites in static CI and `check-all`.
+[Scanner tests](../../../../.github/review-ownership/request-review.test.mjs) cover admitted ownership syntax, rejected syntax, each exclusion class, production-name negative controls, renames, last-match behavior, unmatched files, changed-LOC aggregation and ranking, complete pagination, file and review limits, approval-state reduction, approved-owner suppression and next-owner selection, log-before-mutation ordering, author and existing-reviewer filtering, non-draft reconciliation, draft cancellation provenance, and API failures. [Workflow tests](../../../../scripts/ci-workflow.spec.ts) pin the event set, least permissions, trusted default-branch checkout, absence of pull-request-head references and secrets, and executed command. The gate graph includes both suites in static CI and `check-all`.
 
 
 ## Alternatives considered
 ## Alternatives considered
 
 
@@ -40,12 +42,14 @@ When current review requests exist, the workflow reads the complete review-reque
 
 
 **Cancel every reviewer that no longer matches.** A person may request a reviewer for reasons outside the ownership map. Only requests attributed to the workflow identity are safe for automated reconciliation.
 **Cancel every reviewer that no longer matches.** A person may request a reviewer for reasons outside the ownership map. Only requests attributed to the workflow identity are safe for automated reconciliation.
 
 
+**Treat an empty current request as an owner who still needs review.** GitHub removes the pending request when the reviewer submits a review. Requesting an owner with an active approval again adds no ownership coverage and creates repeated notifications after later synchronize events.
+
 **Infer arbitrary semantic source changes from patches or language parsers.** GitHub can omit or truncate patches, and the repository spans many languages. The scanner does not try to prove that two programs behave identically. The later [comment-only routing decision](2026-09-08-comment-only-review-routing.md) adds a narrow lexical comparison only when changed-line counts prove that GitHub supplied the complete patch.
 **Infer arbitrary semantic source changes from patches or language parsers.** GitHub can omit or truncate patches, and the repository spans many languages. The scanner does not try to prove that two programs behave identically. The later [comment-only routing decision](2026-09-08-comment-only-review-routing.md) adds a narrow lexical comparison only when changed-line counts prove that GitHub supplied the complete patch.
 
 
 ## Consequences
 ## Consequences
 
 
 Reviewer mutations are reproducible from a trusted policy, the file classifications printed in the workflow log, and review-request provenance in the pull-request timeline. Excluded changes do not request owners, rule and changed-file updates remove obsolete workflow-authored requests on the next run, and draft pull requests do not retain workflow-authored requests. Ownership changes become effective only after merge, so the pull request that changes policy cannot apply its untrusted policy to itself.
 Reviewer mutations are reproducible from a trusted policy, the file classifications printed in the workflow log, and review-request provenance in the pull-request timeline. Excluded changes do not request owners, rule and changed-file updates remove obsolete workflow-authored requests on the next run, and draft pull requests do not retain workflow-authored requests. Ownership changes become effective only after merge, so the pull request that changes policy cannot apply its untrusted policy to itself.
 
 
-The workflow requests at most one reviewer per run, keeps no more than one current individual reviewer other than `@turtle1999`, and prefers owners whose matched reviewable files carry more changed LOC. An existing `@turtle1999` request leaves the counted slot available; an existing non-turtle request prevents every additional request. Shared ownership gives each owner the same file-level relevance without counting one renamed file twice for the same owner. GitHub-generated review-request events may not start other workflows that depend on recursively triggered events from `GITHUB_TOKEN`; those workflows must not rely on this request as their only trigger.
+The workflow requests at most one reviewer per run, does not repeat a request while that owner has an active approval, keeps no more than one current individual reviewer other than `@turtle1999`, and prefers owners whose matched reviewable files carry more changed LOC. An existing `@turtle1999` request leaves the counted slot available; an existing non-turtle request prevents every additional request. Shared ownership gives each owner the same file-level relevance without counting one renamed file twice for the same owner. GitHub-generated review-request events may not start other workflows that depend on recursively triggered events from `GITHUB_TOKEN`; those workflows must not rely on this request as their only trigger.
 
 
-Any change that does not match an explicit exclusion remains eligible under an owned directory. Unmatched paths are logged and request nobody. Pull requests above the file or timeline API limit fail without applying a partial reviewer mutation.
+Any change that does not match an explicit exclusion remains eligible under an owned directory. Unmatched paths are logged and request nobody. Pull requests above the file, review, or timeline API limit fail without applying a partial reviewer mutation.

+ 4 - 2
.github/review-ownership/README.md

@@ -20,9 +20,11 @@ Pull requests run the workflow when opened, synchronized, reopened, marked ready
 
 
 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.
 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.
 
 
+Before selecting a new reviewer, a non-draft run fetches the pull request's complete chronological review list. An owner's latest undismissed decisive review is `APPROVED` or `CHANGES_REQUESTED`; comments and pending reviews do not replace that decision. An approved owner remains omitted after later synchronize events, while a later changes-requested review makes the owner eligible again. The workflow fails before mutation when the list reaches the supported 3,000-review limit or contains an invalid record.
+
 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.
 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 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; approved owners omitted from new requests; 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, approved owners, 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.
 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.
 
 
@@ -50,7 +52,7 @@ Ownership changes take effect only after they merge into the default branch. Thi
 
 
 ## Verification
 ## Verification
 
 
-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.
+Run `pnpm run test:request-review` for ownership parsing, file classification, complete-patch checks, comment parsing, changed-LOC ranking, pagination, approval-state reduction, 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>
 <a id="dev-note"></a>
 
 

+ 54 - 1
.github/review-ownership/request-review.mjs

@@ -7,9 +7,11 @@ import { pathToFileURL } from 'node:url'
 const API_VERSION = '2026-03-10'
 const API_VERSION = '2026-03-10'
 const MAX_OWNERS_PER_RULE = 2
 const MAX_OWNERS_PER_RULE = 2
 const MAX_PULL_REQUEST_FILES = 3_000
 const MAX_PULL_REQUEST_FILES = 3_000
+const MAX_PULL_REQUEST_REVIEWS = 3_000
 const MAX_COUNTED_REQUESTED_REVIEWERS = 1
 const MAX_COUNTED_REQUESTED_REVIEWERS = 1
 const MAX_TIMELINE_EVENTS = 3_000
 const MAX_TIMELINE_EVENTS = 3_000
 const PAGE_SIZE = 100
 const PAGE_SIZE = 100
+const PULL_REQUEST_REVIEW_STATES = new Set(['APPROVED', 'CHANGES_REQUESTED', 'COMMENTED', 'DISMISSED', 'PENDING'])
 const UNCOUNTED_REVIEWER = 'turtle1999'
 const UNCOUNTED_REVIEWER = 'turtle1999'
 const WORKFLOW_REVIEW_REQUESTER = 'github-actions[bot]'
 const WORKFLOW_REVIEW_REQUESTER = 'github-actions[bot]'
 const TEST_DIRECTORY_NAMES = new Set(['__snapshots__', '__tests__', 'benches', 'stress-tests', 'test', 'tests'])
 const TEST_DIRECTORY_NAMES = new Set(['__snapshots__', '__tests__', 'benches', 'stress-tests', 'test', 'tests'])
@@ -384,6 +386,47 @@ export async function listPullRequestFiles(api, repository, pullNumber, expected
   return files
   return files
 }
 }
 
 
+/**
+ * Fetch the complete chronological pull-request review list.
+ * @param {(path: string, options?: {method?: string, body?: unknown}) => Promise<unknown>} api GitHub API caller.
+ * @param {string} repository Owner/name repository identifier.
+ * @param {number} pullNumber Pull-request number.
+ * @returns {Promise<unknown[]>} Complete review list within the supported limit.
+ */
+export async function listPullRequestReviews(api, repository, pullNumber) {
+  const reviews = []
+  for (let page = 1; ; page++) {
+    const response = await api(`/repos/${repository}/pulls/${pullNumber}/reviews?per_page=${PAGE_SIZE}&page=${page}`)
+    if (!Array.isArray(response)) throw new Error('pull-request reviews response is not an array')
+    reviews.push(...response)
+    if (response.length < PAGE_SIZE) return reviews
+    if (reviews.length >= MAX_PULL_REQUEST_REVIEWS) {
+      throw new Error(`pull-request reviews exceed ${MAX_PULL_REQUEST_REVIEWS} entries`)
+    }
+  }
+}
+
+/**
+ * Return users whose latest undismissed decisive review approves the pull request.
+ * @param {unknown[]} reviews Chronological GitHub pull-request review records.
+ * @returns {string[]} Approved reviewer logins in stable order.
+ */
+export function approvedReviewerLogins(reviews) {
+  const approved = new Map()
+  for (const review of reviews) {
+    if (!isRecord(review) || !isRecord(review.user) || typeof review.user.login !== 'string') {
+      throw new Error('pull-request reviews response contains an invalid reviewer')
+    }
+    if (typeof review.state !== 'string' || !PULL_REQUEST_REVIEW_STATES.has(review.state)) {
+      throw new Error('pull-request reviews response contains an invalid state')
+    }
+    const key = review.user.login.toLowerCase()
+    if (review.state === 'APPROVED') approved.set(key, review.user.login)
+    else if (review.state === 'CHANGES_REQUESTED') approved.delete(key)
+  }
+  return [...approved.values()].sort((left, right) => left.localeCompare(right, 'en'))
+}
+
 /**
 /**
  * Fetch the pull request timeline used to identify workflow-authored review requests.
  * Fetch the pull request timeline used to identify workflow-authored review requests.
  * @param {(path: string, options?: {method?: string, body?: unknown}) => Promise<unknown>} api GitHub API caller.
  * @param {(path: string, options?: {method?: string, body?: unknown}) => Promise<unknown>} api GitHub API caller.
@@ -472,7 +515,7 @@ export async function requestReviews({ event, ownershipSource, api, write = line
     plan.reviewers.map(({ login, changedLines }) => `@${login}: ${changedLines}`),
     plan.reviewers.map(({ login, changedLines }) => `@${login}: ${changedLines}`),
   )
   )
 
 
-  const candidates = plan.reviewers.filter(({ login }) => login.toLowerCase() !== pull.author.toLowerCase())
+  const ownerCandidates = plan.reviewers.filter(({ login }) => login.toLowerCase() !== pull.author.toLowerCase())
   if (pull.draft) {
   if (pull.draft) {
     const existing = await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`)
     const existing = await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`)
     const requestedReviewers = requestedReviewerLogins(existing)
     const requestedReviewers = requestedReviewerLogins(existing)
@@ -494,6 +537,16 @@ export async function requestReviews({ event, ownershipSource, api, write = line
     return { ...classified, requestedReviewers: [], cancelledReviewers: reviewers }
     return { ...classified, requestedReviewers: [], cancelledReviewers: reviewers }
   }
   }
 
 
+  const approvedReviewerKeys = new Set(
+    (ownerCandidates.length === 0
+      ? []
+      : approvedReviewerLogins(await listPullRequestReviews(api, pull.repository, pull.number)))
+      .map(login => login.toLowerCase()),
+  )
+  const approvedOwners = ownerCandidates.filter(({ login }) => approvedReviewerKeys.has(login.toLowerCase()))
+  const candidates = ownerCandidates.filter(({ login }) => !approvedReviewerKeys.has(login.toLowerCase()))
+  writeList(write, 'Approved owners omitted from review requests', approvedOwners.map(({ login }) => `@${login}`))
+
   const existing = await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`)
   const existing = await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`)
   const currentReviewers = requestedReviewerLogins(existing).sort((left, right) => left.localeCompare(right, 'en'))
   const currentReviewers = requestedReviewerLogins(existing).sort((left, right) => left.localeCompare(right, 'en'))
   const workflowReviewers = currentReviewers.length === 0
   const workflowReviewers = currentReviewers.length === 0

+ 121 - 0
.github/review-ownership/request-review.test.mjs

@@ -4,12 +4,14 @@ import { existsSync, readFileSync } from 'node:fs'
 import test from 'node:test'
 import test from 'node:test'
 
 
 import {
 import {
+  approvedReviewerLogins,
   classifyChangedFiles,
   classifyChangedFiles,
   createGitHubApi,
   createGitHubApi,
   isCommentOnlyChange,
   isCommentOnlyChange,
   isDocumentationPath,
   isDocumentationPath,
   isTestPath,
   isTestPath,
   listPullRequestFiles,
   listPullRequestFiles,
+  listPullRequestReviews,
   listPullRequestTimeline,
   listPullRequestTimeline,
   normalizeRepositoryPath,
   normalizeRepositoryPath,
   parseOwnership,
   parseOwnership,
@@ -333,6 +335,57 @@ test('fails closed when GitHub cannot provide the complete file list', async ()
   )
   )
 })
 })
 
 
+test('fetches pull-request reviews across pages', async () => {
+  const calls = []
+  const pageOne = Array.from({ length: 100 }, (_, index) => ({
+    user: { login: `reviewer-${index}` },
+    state: 'COMMENTED',
+  }))
+  const pageTwo = [{ user: { login: 'approver' }, state: 'APPROVED' }]
+  const reviews = await listPullRequestReviews(async (path) => {
+    calls.push(path)
+    return calls.length === 1 ? pageOne : pageTwo
+  }, 'owner/repo', 42)
+
+  assert.equal(reviews.length, 101)
+  assert.deepEqual(calls, [
+    '/repos/owner/repo/pulls/42/reviews?per_page=100&page=1',
+    '/repos/owner/repo/pulls/42/reviews?per_page=100&page=2',
+  ])
+})
+
+test('tracks each reviewer\'s latest undismissed approval decision', () => {
+  assert.deepEqual(approvedReviewerLogins([
+    { user: { login: 'commented-after' }, state: 'APPROVED' },
+    { user: { login: 'commented-after' }, state: 'COMMENTED' },
+    { user: { login: 'changes-after' }, state: 'APPROVED' },
+    { user: { login: 'changes-after' }, state: 'CHANGES_REQUESTED' },
+    { user: { login: 'dismissed' }, state: 'DISMISSED' },
+    { user: { login: 'approved-after' }, state: 'CHANGES_REQUESTED' },
+    { user: { login: 'approved-after' }, state: 'APPROVED' },
+    { user: { login: 'pending-after' }, state: 'APPROVED' },
+    { user: { login: 'pending-after' }, state: 'PENDING' },
+  ]), ['approved-after', 'commented-after', 'pending-after'])
+
+  assert.throws(
+    () => approvedReviewerLogins([{ user: { login: 'reviewer' }, state: 'UNKNOWN' }]),
+    /invalid state/u,
+  )
+  assert.throws(() => approvedReviewerLogins([{ state: 'APPROVED' }]), /invalid reviewer/u)
+})
+
+test('fails closed when the pull-request review list exceeds its limit', async () => {
+  let calls = 0
+  await assert.rejects(
+    listPullRequestReviews(async () => {
+      calls++
+      return Array.from({ length: 100 }, () => ({ user: { login: 'reviewer' }, state: 'COMMENTED' }))
+    }, 'owner/repo', 42),
+    /exceed 3000 entries/u,
+  )
+  assert.equal(calls, 30)
+})
+
 test('fails closed when the review-request timeline exceeds its limit', async () => {
 test('fails closed when the review-request timeline exceeds its limit', async () => {
   let calls = 0
   let calls = 0
   await assert.rejects(
   await assert.rejects(
@@ -358,6 +411,7 @@ test('prints changed code files and requests the highest-ranked counted owner',
   const api = async (path, options = {}) => {
   const api = async (path, options = {}) => {
     trace.push({ type: 'api', path, options })
     trace.push({ type: 'api', path, options })
     if (path.endsWith('/files?per_page=100&page=1')) return files
     if (path.endsWith('/files?per_page=100&page=1')) return files
+    if (path.endsWith('/reviews?per_page=100&page=1')) return []
     if (path.endsWith('/requested_reviewers') && options.method !== 'POST') {
     if (path.endsWith('/requested_reviewers') && options.method !== 'POST') {
       return { users: [], teams: [] }
       return { users: [], teams: [] }
     }
     }
@@ -409,6 +463,67 @@ test('prints changed code files and requests the highest-ranked counted owner',
   })
   })
 })
 })
 
 
+test('does not request an owner again after that owner approves', async () => {
+  const calls = []
+  const output = []
+  const result = await requestReviews({
+    event: pullRequestEvent(),
+    ownershipSource: '/packages/typert/ @imccyu\n',
+    api: async (path, options = {}) => {
+      calls.push({ path, options })
+      if (path.endsWith('/files?per_page=100&page=1')) {
+        return [{ filename: 'packages/typert/generator/src/analyzer.ts', additions: 150, deletions: 47 }]
+      }
+      if (path.endsWith('/reviews?per_page=100&page=1')) {
+        return [
+          { user: { login: 'imccyu' }, state: 'APPROVED' },
+          { user: { login: 'imccyu' }, state: 'COMMENTED' },
+        ]
+      }
+      if (path.endsWith('/requested_reviewers') && options.method === undefined) {
+        return { users: [], teams: [] }
+      }
+      throw new Error(`unexpected API path ${path}`)
+    },
+    write: line => output.push(line),
+  })
+
+  assert.deepEqual(result.requestedReviewers, [])
+  assert.equal(calls.some(call => call.options.method === 'POST'), false)
+  const approvedHeading = output.indexOf('Approved owners omitted from review requests:')
+  assert.ok(approvedHeading >= 0)
+  assert.equal(output[approvedHeading + 1], '- @imccyu')
+})
+
+test('fills the counted slot with the next owner after omitting an approved owner', async () => {
+  const calls = []
+  const result = await requestReviews({
+    event: pullRequestEvent(),
+    ownershipSource: '/packages/core/ @imccyu @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('/reviews?per_page=100&page=1')) {
+        return [{ user: { login: 'imccyu' }, state: 'APPROVED' }]
+      }
+      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, ['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 add another counted owner when one is already requested', async () => {
 test('does not add another counted owner when one is already requested', async () => {
   const calls = []
   const calls = []
   const output = []
   const output = []
@@ -420,6 +535,7 @@ test('does not add another counted owner when one is already requested', async (
       if (path.endsWith('/files?per_page=100&page=1')) {
       if (path.endsWith('/files?per_page=100&page=1')) {
         return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
         return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
       }
       }
+      if (path.endsWith('/reviews?per_page=100&page=1')) return []
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
         return { users: [{ login: 'first' }], teams: [] }
         return { users: [{ login: 'first' }], teams: [] }
       }
       }
@@ -455,6 +571,7 @@ test('requests at most one owner per run when turtle ranks first', async () => {
           { filename: 'packages/client/store/src/index.ts', additions: 8, deletions: 2 },
           { filename: 'packages/client/store/src/index.ts', additions: 8, deletions: 2 },
         ]
         ]
       }
       }
+      if (path.endsWith('/reviews?per_page=100&page=1')) return []
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
         return { users: [], teams: [] }
         return { users: [], teams: [] }
       }
       }
@@ -481,6 +598,7 @@ test('does not add turtle when one counted reviewer is already requested', async
       if (path.endsWith('/files?per_page=100&page=1')) {
       if (path.endsWith('/files?per_page=100&page=1')) {
         return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
         return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
       }
       }
+      if (path.endsWith('/reviews?per_page=100&page=1')) return []
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
         return { users: [{ login: 'first' }], teams: [] }
         return { users: [{ login: 'first' }], teams: [] }
       }
       }
@@ -504,6 +622,7 @@ test('keeps the counted slot available when turtle is already requested', async
       if (path.endsWith('/files?per_page=100&page=1')) {
       if (path.endsWith('/files?per_page=100&page=1')) {
         return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
         return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
       }
       }
+      if (path.endsWith('/reviews?per_page=100&page=1')) return []
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
         return { users: [{ login: 'turtle1999' }], teams: [] }
         return { users: [{ login: 'turtle1999' }], teams: [] }
       }
       }
@@ -531,6 +650,7 @@ test('replaces a workflow reviewer that no longer matches current ownership', as
       if (path.endsWith('/files?per_page=100&page=1')) {
       if (path.endsWith('/files?per_page=100&page=1')) {
         return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
         return [{ filename: 'packages/core/agent/src/index.ts', additions: 20, deletions: 10 }]
       }
       }
+      if (path.endsWith('/reviews?per_page=100&page=1')) return []
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
         return { users: [{ login: 'Dudu-0223' }], teams: [] }
         return { users: [{ login: 'Dudu-0223' }], teams: [] }
       }
       }
@@ -583,6 +703,7 @@ test('removes excess workflow reviewers using current relevance order', async ()
           { filename: 'packages/subagent/subagent/src/index.ts', additions: 8, deletions: 2 },
           { filename: 'packages/subagent/subagent/src/index.ts', additions: 8, deletions: 2 },
         ]
         ]
       }
       }
+      if (path.endsWith('/reviews?per_page=100&page=1')) return []
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
       if (path.endsWith('/requested_reviewers') && options.method === undefined) {
         return { users: [{ login: 'Dudu-0223' }, { login: 'mektpoy' }], teams: [] }
         return { users: [{ login: 'Dudu-0223' }, { login: 'mektpoy' }], teams: [] }
       }
       }