Jelajahi Sumber

Merge pull request #3845 from deepseek-harness/turtle/keep-blocked-approval-pending

Keep blocked weighted approvals pending
Turtle 1 Minggu lalu
induk
melakukan
d6a699be19

+ 6 - 0
.agents/notes/implemented/process/2026-09-09-blocked-weighted-approvals-remain-pending.i18n.yaml

@@ -0,0 +1,6 @@
+# Bilingual-pair consistency record (docs/i18n/README.md): the git blob hash of each
+# side as of the last confirmed-consistent state. Both languages carry equal authority;
+# after editing either side, bring the other along and re-record with:
+#   pnpm run verify-translation-pairing --write .agents/notes/implemented/process/2026-09-09-blocked-weighted-approvals-remain-pending.md
+2026-09-09-blocked-weighted-approvals-remain-pending.md: 50ab64dadc0d7ad14366950d24b4136d131f55ec
+2026-09-09-blocked-weighted-approvals-remain-pending.zh.md: 58ab10b3acb3bee92c720b622d92170a1a8bcf03

+ 33 - 0
.agents/notes/implemented/process/2026-09-09-blocked-weighted-approvals-remain-pending.md

@@ -0,0 +1,33 @@
+# Agent Note: Blocked weighted approvals remain pending
+
+Status: implemented
+
+English | [中文](2026-09-09-blocked-weighted-approvals-remain-pending.zh.md)
+
+## Problem
+
+The weighted approval commit status must distinguish an unmet merge condition from a failed policy evaluation. An effective `CHANGES_REQUESTED` review from a write-capable reviewer prevents a pull request from satisfying the approval policy, but it is a reversible review state rather than an evaluation failure.
+
+Publishing `failure` for that review state conflates the approval decision with the health of the publisher. It also treats one unmet policy condition differently from a draft pull request or insufficient approval points, which remain pending while contributors can resolve them.
+
+## Decision
+
+A completed weighted approval evaluation publishes `pending` when the pull request is a draft, has fewer than the required approval points, or has an effective `CHANGES_REQUESTED` review from a write-capable reviewer. A blocking review dominates the point total, so the status remains pending even when counted approvals reach the threshold.
+
+The evaluation publishes `success` only when the pull request is ready, the point threshold is met, and no blocking review exists. The separate `weighted approval publisher` Actions job reports whether evaluation and status publication completed. An evaluation failure publishes an `error` commit status and fails that job.
+
+## Verification
+
+[Approval policy tests](../../../../.github/review-ownership/check-approval.test.mjs) pin the threshold-reaching blocker case and the exact published `pending` payload. [Workflow tests](../../../../scripts/ci-workflow.spec.ts) pin the separate publisher job name.
+
+## Alternatives considered
+
+**Publish `failure` for a blocking review.** This keeps a visibly failed status until the review changes, but it represents an unmet and reversible merge condition as a malfunction and conflates policy outcome with publisher health.
+
+**Let approval points override a blocking review.** This makes the score the only success condition, but it permits a successful status while a write-capable reviewer's effective decision still requests changes.
+
+## Consequences
+
+Required-status branch rules block a pull request because `pending` does not satisfy the required status. Contributors can distinguish review work that remains from a failed approval evaluation, while the publisher job and `error` status retain the operational failure signal.
+
+Consumers do not receive a failed commit status solely because a blocking review exists. They must inspect the status description or effective reviews when they need to distinguish a blocker from other pending approval conditions.

+ 33 - 0
.agents/notes/implemented/process/2026-09-09-blocked-weighted-approvals-remain-pending.zh.md

@@ -0,0 +1,33 @@
+# Agent Note: 阻塞中的加权批准保持 pending
+
+Status: implemented
+
+[English](2026-09-09-blocked-weighted-approvals-remain-pending.md) | 中文
+
+## 问题
+
+`weighted approval` commit status 必须区分尚未满足的合并条件与失败的策略评估。具有写权限的评审人所提交且仍然生效的 `CHANGES_REQUESTED` 评审会阻止 PR 满足批准策略,但它是一种可撤销的评审状态,而不是评估故障。
+
+为这种评审状态发布 `failure` 会混淆批准决策与 publisher 的健康状态。它还会让一个尚未满足的策略条件区别于 draft PR 或批准点数不足;后两种情况在贡献者能够解决问题期间会保持 pending。
+
+## 决策
+
+完成的加权批准评估会在 PR 为 draft、批准点数少于要求,或具有写权限的评审人存在仍然生效的 `CHANGES_REQUESTED` 评审时发布 `pending`。阻塞性评审的优先级高于点数总和,因此即使计入的批准已经达到阈值,状态仍保持 pending。
+
+只有在 PR 已进入 ready 状态、达到点数阈值且不存在阻塞性评审时,评估才发布 `success`。独立的 `weighted approval publisher` Actions job 报告评估和状态发布是否完成。评估故障会发布 `error` commit status,并使该 job 失败。
+
+## 验证
+
+[批准策略测试](../../../../.github/review-ownership/check-approval.test.mjs)锁定已达到阈值但仍有 blocker 的场景,以及准确发布的 `pending` payload。[工作流测试](../../../../scripts/ci-workflow.spec.ts)锁定独立的 publisher job 名称。
+
+## 考虑过的替代方案
+
+**为阻塞性评审发布 `failure`。** 该方案会在评审改变之前保持明显的失败状态,但它会把尚未满足且可撤销的合并条件表示为故障,并混淆策略结果与 publisher 的健康状态。
+
+**允许批准点数覆盖阻塞性评审。** 该方案会让分数成为唯一的成功条件,但也允许在具有写权限的评审人仍然有效地要求修改时发布成功状态。
+
+## 后果
+
+需要该状态的分支规则会阻止 PR,因为 `pending` 不满足必需状态。贡献者可以区分尚待处理的评审工作与失败的批准评估,而 publisher job 和 `error` 状态保留运行故障信号。
+
+消费方不会仅因存在阻塞性评审而收到失败的 commit status。如果需要区分 blocker 与其他 pending 批准条件,它们必须检查状态描述或仍然生效的评审。

+ 1 - 1
.github/review-ownership/README.md

@@ -15,7 +15,7 @@ The [`weighted-approval` workflow](../workflows/weighted-approval.yml) publishes
 
 ## Approval scoring
 
-The weighted approval workflow publishes the `weighted approval` commit status on the pull request head. Branch rules must require this status with GitHub Actions as its expected source; a context-only requirement can accept a same-named status from another integration. The status succeeds at two approval points, remains pending below two points or while the pull request is a draft, fails while a write-capable reviewer has an effective `CHANGES_REQUESTED` review, and reports an error when policy evaluation fails.
+The weighted approval workflow exposes two pull-request checks. The `weighted approval publisher` Actions job reports whether evaluation and status publication completed, while the `weighted approval` commit status carries the approval decision on the pull request head. Branch rules must require only the commit status with GitHub Actions as its expected source; a context-only requirement can accept a same-named status from another integration. A completed evaluation returns `pending` below two approval points, while the pull request is a draft, or while a write-capable reviewer has an effective `CHANGES_REQUESTED` review; the blocker keeps the status pending even when counted approvals reach the threshold. It returns `success` only when the threshold is met, the pull request is ready, and no such blocker exists. If evaluation fails, the publisher writes an `error` status.
 
 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 and reviewers without write permission do not count.
 

+ 2 - 2
.github/review-ownership/check-approval.mjs

@@ -129,7 +129,7 @@ export async function listPullRequestReviews(api, repository, pullNumber) {
 /**
  * Evaluate approval points from current reviews and repository permissions.
  * @param {{event: unknown, policySource: string, api: (path: string, options?: {method?: string, body?: unknown}) => Promise<unknown>}} options Runtime inputs.
- * @returns {Promise<{pull: {repository: string, number: number, headSha: string}, state: 'failure' | 'pending' | 'success', description: string, points: number, requiredPoints: number, approvals: Array<{login: string, points: number}>, blockers: string[], ignoredReviewers: string[]}>} Approval decision and status payload fields.
+ * @returns {Promise<{pull: {repository: string, number: number, headSha: string}, state: 'pending' | 'success', description: string, points: number, requiredPoints: number, approvals: Array<{login: string, points: number}>, blockers: string[], ignoredReviewers: string[]}>} Approval decision and status payload fields.
  */
 export async function evaluateApproval({ event, policySource, api }) {
   const pull = pullRequestFromEvent(event)
@@ -169,7 +169,7 @@ export async function evaluateApproval({ event, policySource, api }) {
     return next
   }, 0)
   if (blockers.length > 0) {
-    return approvalResult(pull, policy.requiredPoints, approvals, blockers, ignoredReviewers, 'failure',
+    return approvalResult(pull, policy.requiredPoints, approvals, blockers, ignoredReviewers, 'pending',
       `${blockers.length} blocking change request${blockers.length === 1 ? '' : 's'}`)
   }
   const state = points >= policy.requiredPoints ? 'success' : 'pending'

+ 18 - 4
.github/review-ownership/check-approval.test.mjs

@@ -206,11 +206,13 @@ test('ignores a reviewer whose collaborator permission lookup returns 404', asyn
   assert.deepEqual(result.ignoredReviewers, ['former-writer'])
 })
 
-test('blocks on a write-capable change request but ignores the author and read-only blockers', async () => {
-  const result = await evaluateApproval({
+test('keeps the status pending on a write-capable change request while ignoring the author and read-only reviewers', async () => {
+  const statuses = []
+  const result = await runApprovalCheck({
     event: pullRequestEvent({ author: 'author' }),
     policySource,
-    api: async (path) => {
+    runUrl: 'https://github.example/actions/runs/1',
+    api: async (path, options = {}) => {
       if (path.includes('/reviews?')) {
         return [
           review('turtle1999', 'APPROVED'),
@@ -222,13 +224,25 @@ test('blocks on a write-capable change request but ignores the author and read-o
       if (path.includes('/collaborators/turtle1999/permission')) return { permission: 'admin' }
       if (path.includes('/collaborators/blocker/permission')) return { permission: 'write' }
       if (path.includes('/collaborators/reader/permission')) return { permission: 'read' }
+      if (path.includes('/statuses/')) {
+        statuses.push(options.body)
+        return {}
+      }
       throw new Error(`unexpected API path ${path}`)
     },
+    write: () => {},
   })
-  assert.equal(result.state, 'failure')
+  assert.equal(result.state, 'pending')
+  assert.equal(result.description, '1 blocking change request.')
   assert.equal(result.points, 2)
   assert.deepEqual(result.blockers, ['blocker'])
   assert.deepEqual(result.ignoredReviewers, ['reader'])
+  assert.deepEqual(statuses, [{
+    state: 'pending',
+    context: 'weighted approval',
+    description: '1 blocking change request.',
+    target_url: 'https://github.example/actions/runs/1',
+  }])
 })
 
 test('keeps drafts pending without reading reviews', async () => {

+ 1 - 1
.github/workflows/weighted-approval.yml

@@ -19,7 +19,7 @@ concurrency:
 jobs:
   publish-status:
     if: github.event_name != 'workflow_run' || github.event.workflow_run.conclusion == 'success'
-    name: publish weighted approval status
+    name: weighted approval publisher
     runs-on: ubuntu-latest
     timeout-minutes: 5
     steps:

+ 1 - 1
scripts/ci-workflow.spec.ts

@@ -813,7 +813,7 @@ describe('Weighted approval workflow', () => {
     })
     expect(job).toMatchObject({
       if: "github.event_name != 'workflow_run' || github.event.workflow_run.conclusion == 'success'",
-      name: 'publish weighted approval status',
+      name: 'weighted approval publisher',
       'runs-on': 'ubuntu-latest',
       'timeout-minutes': 5,
     })