diff --git a/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.i18n.yaml b/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.i18n.yaml index 674e8e13c6..aea251ed42 100644 --- a/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.i18n.yaml +++ b/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.i18n.yaml @@ -9,14 +9,14 @@ en: 6ba6f6a15a1562ec zh: 1cbef3e736c258e6 /agent-note-weighted-approval-decisions-are-completed-check-runs/decision: - en: b070a25a6d990e8c - zh: fa555f8e418dad1f + en: 7932e500d3fb794f + zh: 576fffbf7525c12b /agent-note-weighted-approval-decisions-are-completed-check-runs/verification: - en: 0cd6f1b14b31db64 - zh: 7fc266aa0006473d + en: f0b7fb23bab7d23c + zh: d67e23a22b763bae /agent-note-weighted-approval-decisions-are-completed-check-runs/alternatives-considered: en: b8daea66b0a90d26 zh: fb7079c815b38281 /agent-note-weighted-approval-decisions-are-completed-check-runs/consequences: - en: 72881a2a72c37e4e - zh: b69bd79b0bb46edb + en: d39c80f2e8c802ee + zh: 4e72296a9495d02c diff --git a/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.md b/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.md index 545c051b75..b78e25827b 100644 --- a/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.md +++ b/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.md @@ -22,15 +22,15 @@ The [publisher](../../../../.github/review-ownership/check-approval.mjs) reports | Ready, threshold met, and no blocking review | `completed`, conclusion `success` | | Setup, evaluation, or cancellation failure | `completed`, conclusion `failure` | -A check run that reached `completed` never returns to `in_progress` or `queued`: GitHub answers `200` to that update and leaves the run completed, so only the conclusion is mutable. The publisher therefore revokes a published success by writing the blocking conclusion before dependency setup, which covers the setup and history fetch that dominate a cold evaluation. The head keeps one GitHub Actions run with the required name: the publisher updates the head's existing run and creates one only when none exists. An incomplete list or a run without a valid ID fails the publication. A run cancelled after the decision was published keeps that decision; every other failed or cancelled run concludes `failure`. +A check run that reached `completed` never returns to `in_progress` or `queued`: GitHub answers `200` to that update and leaves the run completed, so only the conclusion is mutable. The publisher therefore revokes a published success by writing the blocking conclusion before dependency setup, which covers the setup and history fetch that dominate a cold evaluation. The head keeps one GitHub Actions run with the required name: the publisher updates the head's existing run and creates one only when none exists. An incomplete list or a run without a valid ID fails the publication. The publish step records the conclusion it wrote in a step output, and the setup-failure step runs only when no conclusion was recorded: a published decision survives a later failure or cancellation, while a run cancelled before that write concludes `failure`. During a re-evaluation, the earlier decision's conclusion and title stay visible until the new decision is written. A blocking review dominates the point total, so the conclusion remains `action_required` even when counted approvals reach the threshold. GitHub accepts only `success`, `neutral`, and `skipped` for a required check, so `action_required` blocks merging while completing the check. ## Verification -[Approval policy tests](../../../../.github/review-ownership/check-approval.test.mjs) pin the threshold-reaching blocker case, the exact published check-run payloads, revoking a published success through its conclusion, updating an existing run instead of creating another, keeping the evaluation failure title when the setup-failure step follows it, and invalid list responses. [Workflow tests](../../../../scripts/ci-workflow.spec.ts) pin the `checks: write` permission, the `revoke` and `failure` phase commands, the publish step identity, and the fallback that runs when a run fails or is cancelled before publishing. +[Approval policy tests](../../../../.github/review-ownership/check-approval.test.mjs) pin the threshold-reaching blocker case, the exact published check-run payloads, revoking a published success through its conclusion, updating an existing run instead of creating another, recording the conclusion that the setup-failure step reads, and invalid list responses. [Workflow tests](../../../../scripts/ci-workflow.spec.ts) pin the `checks: write` permission, the `revoke` and `failure` phase commands, the publish step identity, and the fallback that runs when a run fails or is cancelled before publishing. -The transition matrix and the fallback condition were measured on 2026-09-30 with throwaway tag runs in this repository, using the same GitHub Actions token as the publisher. A run created as `in_progress` and completed as `success` accepted `status: in_progress` and `status: queued` with `200` and stayed `completed`, while conclusion-only updates moved it to `action_required`, back to `success`, and to `failure`. The fallback step ran for a failed publish and for a cancellation during the publish step, and did not run when the job was cancelled after the decision was published. +The transition matrix and the fallback condition were measured on 2026-09-30 with throwaway tag runs in this repository, using the same GitHub Actions token as the publisher. A run created as `in_progress` and completed as `success` accepted `status: in_progress` and `status: queued` with `200` and stayed `completed`, while conclusion-only updates moved it to `action_required`, back to `success`, and to `failure`. The fallback step ran for a failed publish and for a cancellation during the publish step, and did not run when the job was cancelled after the decision was published. A `timeout-minutes` cancellation was measured separately on 2026-10-20: a job that exceeded its one-minute timeout on a throwaway `push` branch still ran its `cancelled()` step, so the setup-failure step executes on that path too. The list response's `total_count` counts the `check_name`-filtered runs rather than every check on the head: on 2026-10-20 a head carrying 33 check runs reported `total_count` 0 for a name that matches none and 2 for an existing name, so an equality between `total_count` and the returned array length rejects a truncated page. @@ -56,4 +56,4 @@ CI watchers finish while the decision waits on reviewers. `gh pr checks` groups Re-evaluating a head that already published `success` reports `action_required` until the new decision is written, so a watcher that polls inside that window sees a failed check. -The branch-rule switch leaves two gaps on heads the publisher has not evaluated since it. A head without the decision check run reports the required check as expected until a push, a review, or a re-run of its latest publisher run creates it. A head that also carries the earlier `weighted approval` commit status keeps that pending status until the same event: the combined status resolves each context to its latest status, so the rollout posts a terminal `success` status for that context on every open head instead of waiting for its next push. +The branch-rule switch leaves two gaps on heads the publisher has not evaluated since it. A head without the decision check run reports the required check as expected until the next publisher event creates it; re-running a pre-switch run does not, because a re-run replays that run's earlier workflow file, which calls the removed `pending` phase against the new script. A re-run of a pre-switch run on 2026-10-20 executed the pre-switch step names and no check-run write. A head that also carries the earlier `weighted approval` commit status keeps that pending status until the same event: the combined status resolves each context to its latest status, so the rollout posts a terminal `success` status for that context on every open head instead of waiting for its next push. diff --git a/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.zh.md b/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.zh.md index 18227898a9..8a12d784a6 100644 --- a/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.zh.md +++ b/.agents/notes/implemented/process/2026-09-29-weighted-approval-decision-check-run.zh.md @@ -22,15 +22,15 @@ commit status 只提供 `pending`、`success`、`failure` 和 `error`。`pending | 已 ready、达到阈值且没有阻塞性评审 | `completed`,conclusion 为 `success` | | 安装、评估或取消导致的失败 | `completed`,conclusion 为 `failure` | -已经到达 `completed` 的 check run 不会再回到 `in_progress` 或 `queued`:GitHub 对该更新返回 `200`,但让 run 保持已完成,因此只有 conclusion 可变。publisher 因此在依赖安装前用阻塞性 conclusion 撤销已发布的成功,覆盖冷启动中占主要耗时的安装与历史拉取。同一 head 上只保留一个同名 GitHub Actions run:publisher 更新 head 上已有的 run,仅在不存在时创建一个。列表不完整或 run 没有有效 ID 时发布失败。决策已发布后被取消的 run 保留该决策;其余失败或被取消的 run 都以 `failure` 结束。 +已经到达 `completed` 的 check run 不会再回到 `in_progress` 或 `queued`:GitHub 对该更新返回 `200`,但让 run 保持已完成,因此只有 conclusion 可变。publisher 因此在依赖安装前用阻塞性 conclusion 撤销已发布的成功,覆盖冷启动中占主要耗时的安装与历史拉取。同一 head 上只保留一个同名 GitHub Actions run:publisher 更新 head 上已有的 run,仅在不存在时创建一个。列表不完整或 run 没有有效 ID 时发布失败。publish 步骤把自己写入的 conclusion 记录到 step output,安装失败步骤只在该 output 未记录时运行:已发布的决策在之后的失败或取消中保留,而在该写入之前被取消的 run 以 `failure` 结束。重新评估期间,先前的 conclusion 与标题会一直显示到新决策写入为止。 阻塞性评审的优先级高于点数总和,因此即使计入的批准已经达到阈值,conclusion 仍为 `action_required`。GitHub 只接受 `success`、`neutral` 和 `skipped` 作为必需检查的通过结果,因此 `action_required` 会阻止合并,同时让该检查处于完成状态。 ## 验证 -[批准策略测试](../../../../.github/review-ownership/check-approval.test.mjs)锁定已达到阈值但仍有 blocker 的场景、准确发布的 check run payload、通过 conclusion 撤销已发布的成功、更新已有 run 而不是另建一个、安装失败步骤跟在评估失败之后时保留该标题,以及无效的列表响应。[工作流测试](../../../../scripts/ci-workflow.spec.ts)锁定 `checks: write` 权限、`revoke` 和 `failure` 阶段命令、publish 步骤的身份,以及运行失败或在发布前被取消时执行的兜底步骤。 +[批准策略测试](../../../../.github/review-ownership/check-approval.test.mjs)锁定已达到阈值但仍有 blocker 的场景、准确发布的 check run payload、通过 conclusion 撤销已发布的成功、更新已有 run 而不是另建一个、记录安装失败步骤所读的 conclusion,以及无效的列表响应。[工作流测试](../../../../scripts/ci-workflow.spec.ts)锁定 `checks: write` 权限、`revoke` 和 `failure` 阶段命令、publish 步骤的身份,以及运行失败或在发布前被取消时执行的兜底步骤。 -转换矩阵与该兜底条件于 2026-09-30 在本仓库用一次性 tag 运行实测,使用的是与 publisher 相同的 GitHub Actions token。以 `in_progress` 创建、以 `success` 完成的 run,接受 `status: in_progress` 和 `status: queued` 的更新时都返回 `200` 并保持 `completed`;只改 conclusion 的更新把它依次改为 `action_required`、`success` 和 `failure`。兜底步骤在发布失败以及在发布步骤执行期间被取消时运行,而在决策发布之后才取消 job 时不运行。 +转换矩阵与该兜底条件于 2026-09-30 在本仓库用一次性 tag 运行实测,使用的是与 publisher 相同的 GitHub Actions token。以 `in_progress` 创建、以 `success` 完成的 run,接受 `status: in_progress` 和 `status: queued` 的更新时都返回 `200` 并保持 `completed`;只改 conclusion 的更新把它依次改为 `action_required`、`success` 和 `failure`。兜底步骤在发布失败以及在发布步骤执行期间被取消时运行,而在决策发布之后才取消 job 时不运行。`timeout-minutes` 取消路径于 2026-10-20 单独实测:一个在一次性 `push` 分支上超过一分钟超时的 job 仍然执行了 `cancelled()` 步骤,因此安装失败步骤在该路径上同样会运行。 列表响应中的 `total_count` 统计的是经 `check_name` 过滤后的 run,而不是该 head 上的全部检查:2026-10-20 实测,一个带有 33 个 check run 的 head,对不匹配任何 run 的名称返回 `total_count` 0,对已存在的名称返回 2,因此用 `total_count` 与返回数组长度相等来拒绝被截断的分页是有效的。 @@ -56,4 +56,4 @@ commit status 只提供 `pending`、`success`、`failure` 和 `error`。`pending 重新评估已经发布 `success` 的 head 时,在新决策写入前会报告 `action_required`,因此在该窗口内轮询的监视工具会看到失败的检查。 -分支规则切换后,对切换以来 publisher 尚未评估过的 head 存在两个缺口。没有该决策 check run 的 head 会把必需检查报告为 expected,直到一次 push、一次评审,或重跑其最新的 publisher run 创建它。同时也带有先前 `weighted approval` commit status 的 head 会一直保留那个 pending status 直到同一事件:combined status 对每个 context 取最新的 status,因此 rollout 对每个开放 head 发布该 context 的终态 `success` status,而不是等它下一次 push。 +分支规则切换后,对切换以来 publisher 尚未评估过的 head 存在两个缺口。没有该决策 check run 的 head 会把必需检查报告为 expected,直到下一次 publisher 事件创建它;重跑切换前的 run 不能完成迁移,因为重跑会回放该 run 当时的 workflow 文件,而它调用的 `pending` 阶段已被新脚本拒绝。2026-10-20 对切换前 run 的重跑执行的是切换前的步骤名,没有写入 check run。同时也带有先前 `weighted approval` commit status 的 head 会一直保留那个 pending status 直到同一事件:combined status 对每个 context 取最新的 status,因此 rollout 对每个开放 head 发布该 context 的终态 `success` status,而不是等它下一次 push。 diff --git a/.agents/skills/dsh-pre-push-checks/SKILL.md b/.agents/skills/dsh-pre-push-checks/SKILL.md index ec49e2b223..838e93e0b3 100644 --- a/.agents/skills/dsh-pre-push-checks/SKILL.md +++ b/.agents/skills/dsh-pre-push-checks/SKILL.md @@ -124,7 +124,7 @@ For GitHub PRs, inspect remote CI after the push: gh pr checks ``` -Report pending checks as pending. Inspect failures before attributing them to the branch or the environment. A `weighted approval decision` check that `gh pr checks` lists as failed means the approval gate is waiting on reviewers (`ACTION_REQUIRED`), not that the branch broke. +Report pending checks as pending. Inspect failures before attributing them to the branch or the environment. A `weighted approval decision` check that `gh pr checks` lists as failed means the approval gate is waiting on reviewers only when its conclusion is `ACTION_REQUIRED`; the same check concluded `FAILURE` is a publisher fault, so read the `weighted approval publisher` job before attributing it to the branch. When `gh pr checks` reports "no checks reported" and `/actions/runs?head_sha=` returns `total_count: 0`, read mergeability before suspecting the push or a dropped GitHub event: diff --git a/.github/review-ownership/check-approval.mjs b/.github/review-ownership/check-approval.mjs index 55de5a7c12..52cd3276e4 100644 --- a/.github/review-ownership/check-approval.mjs +++ b/.github/review-ownership/check-approval.mjs @@ -331,6 +331,7 @@ export async function runApprovalCheck({ event, policySource, api, runUrl, getOw } } catch (error) { await publishCheck(api, pull, 'failure', 'Approval evaluation failed.', runUrl) + recordConclusion('failure') throw error } write(result.authorCredit @@ -342,6 +343,7 @@ export async function runApprovalCheck({ event, policySource, api, runUrl, getOw writeList(write, 'Blocking change requests', result.blockers.map(login => `@${login}`)) writeList(write, 'Ignored reviewers without write access', result.ignoredReviewers.map(login => `@${login}`)) await publishCheck(api, pull, result.state, result.description, runUrl) + recordConclusion(result.state) write(`Published ${JSON.stringify(CHECK_NAME)} check run ${JSON.stringify(result.state)}.`) try { await requestDelegatedReviews(result, api, write) @@ -359,12 +361,7 @@ export async function runApprovalCheck({ event, policySource, api, runUrl, getOw export async function publishApprovalPhase({ event, api, runUrl, phase }) { const pull = pullRequestFromEvent(event) if (phase === 'revoke') return revokeApproval(api, pull, runUrl) - // The setup-failure step runs after an evaluation failure already concluded this run as failure - // with a title that names what failed; keep that write instead of replacing its title. - if (phase === 'failure') { - return publishCheck(api, pull, 'failure', 'Approval setup or evaluation failed.', runUrl, - run => run.conclusion === 'failure' && run.details_url === runUrl) - } + if (phase === 'failure') return publishCheck(api, pull, 'failure', 'Approval setup or evaluation failed.', runUrl) throw new Error('invalid approval setup phase') } @@ -474,7 +471,7 @@ async function reviewerPermission(api, repository, login) { // One decision per head keeps branch rules and CI watchers unambiguous; the head's existing GitHub // Actions run is updated and a run is created only when none exists. -async function publishCheck(api, pull, conclusion, title, runUrl, keep = () => false) { +async function publishCheck(api, pull, conclusion, title, runUrl) { requireHttpsRunUrl(runUrl) const body = { status: 'completed', conclusion, details_url: runUrl, output: { title, summary: `${title} [Workflow run](${runUrl})` } } const runs = await decisionRuns(api, pull) @@ -482,10 +479,9 @@ async function publishCheck(api, pull, conclusion, title, runUrl, keep = () => f await api(`/repos/${pull.repository}/check-runs`, { method: 'POST', body: { name: CHECK_NAME, head_sha: pull.headSha, ...body } }) return } - for (const run of runs) { - if (!Number.isSafeInteger(run.id)) throw new Error('check run has no valid id') - if (keep(run)) continue - await api(`/repos/${pull.repository}/check-runs/${run.id}`, { method: 'PATCH', body }) + for (const { id } of runs) { + if (!Number.isSafeInteger(id)) throw new Error('check run has no valid id') + await api(`/repos/${pull.repository}/check-runs/${id}`, { method: 'PATCH', body }) } } @@ -505,7 +501,8 @@ async function revokeApproval(api, pull, runUrl) { } for (const { id, conclusion } of runs) { if (!Number.isSafeInteger(id)) throw new Error('check run has no valid id') - // Only a published success satisfies the branch rule; every other conclusion already blocks it. + // This publisher concludes a run success only when the policy is met, so a published success is + // the only conclusion here that satisfies the branch rule; it never writes neutral or skipped. if (conclusion !== 'success') continue await api(`/repos/${pull.repository}/check-runs/${id}`, { method: 'PATCH', @@ -524,6 +521,12 @@ async function decisionRuns(api, pull) { return listed.check_runs.filter(run => isRecord(run) && isRecord(run.app) && run.app.slug === 'github-actions') } +// The setup-failure step reads this output so it replaces a conclusion only when this run did not +// already publish one; a cancelled run cannot rely on the step outcome for that distinction. +function recordConclusion(conclusion) { + if (process.env.GITHUB_OUTPUT) appendFileSync(process.env.GITHUB_OUTPUT, `concluded=${conclusion}\n`) +} + function requireHttpsRunUrl(runUrl) { if (!/^https:\/\/[^\s]+$/u.test(runUrl)) throw new Error('workflow run URL must use HTTPS') } diff --git a/.github/review-ownership/check-approval.test.mjs b/.github/review-ownership/check-approval.test.mjs index f406b03dc4..ab279b7e62 100644 --- a/.github/review-ownership/check-approval.test.mjs +++ b/.github/review-ownership/check-approval.test.mjs @@ -1,5 +1,7 @@ import assert from 'node:assert/strict' -import { readFileSync } from 'node:fs' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import test from 'node:test' import { @@ -531,24 +533,42 @@ test('updates the head\'s GitHub Actions decision check run instead of creating } }) -test('setup failure keeps the evaluation failure title published for the same run', async () => { - const calls = [] - const checkRuns = [ - { id: 7, app: { slug: 'github-actions' }, conclusion: 'failure', details_url: 'https://github.example/run/1' }, - { id: 8, app: { slug: 'github-actions' }, conclusion: 'failure', details_url: 'https://github.example/old-run' }, - { id: 9, app: { slug: 'github-actions' }, conclusion: 'action_required', details_url: 'https://github.example/run/1' }, - ] - await publishApprovalPhase({ - event: pullRequestEvent(), runUrl: 'https://github.example/run/1', phase: 'failure', - api: async (path, options) => { - if (!isCheckRunsList(path)) calls.push({ path, method: options?.method }) - return isCheckRunsList(path) ? { total_count: checkRuns.length, check_runs: checkRuns } : {} - }, - }) - assert.deepEqual(calls, [ - { path: '/repos/deepseek-harness/deepseek-harness/check-runs/8', method: 'PATCH' }, - { path: '/repos/deepseek-harness/deepseek-harness/check-runs/9', method: 'PATCH' }, - ]) +test('records the concluded decision so the failure step never replaces it', async () => { + const directory = mkdtempSync(join(tmpdir(), 'approval-output-')) + const output = join(directory, 'output') + writeFileSync(output, '') + const previous = process.env.GITHUB_OUTPUT + process.env.GITHUB_OUTPUT = output + const api = async (path, options) => { + if (isCheckRunsList(path)) return emptyCheckRuns + if (path.includes('/reviews?')) return [review('turtle2099', 'APPROVED')] + if (path.includes('/permission')) return { permission: 'write' } + if (path.includes('/comments?')) return [] + assert.match(path, /\/check-runs$/u) + return {} + } + try { + await runApprovalCheck({ + event: pullRequestEvent(), policySource, runUrl: 'https://github.example/run/1', api, write: () => {}, + }) + assert.match(readFileSync(output, 'utf8'), /^concluded=success$/mu) + writeFileSync(output, '') + await assert.rejects(runApprovalCheck({ + event: pullRequestEvent(), policySource, runUrl: 'https://github.example/run/2', write: () => {}, + getMergedCount: async () => { throw new Error('history unavailable') }, + api: async path => { + if (isCheckRunsList(path)) return emptyCheckRuns + if (path.includes('/reviews?')) return [] + if (path.includes('/comments?')) return [] + return {} + }, + }), /history unavailable/u) + assert.match(readFileSync(output, 'utf8'), /^concluded=failure$/mu) + } finally { + if (previous === undefined) delete process.env.GITHUB_OUTPUT + else process.env.GITHUB_OUTPUT = previous + rmSync(directory, { recursive: true, force: true }) + } }) 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]]) { diff --git a/.github/workflows/weighted-approval.yml b/.github/workflows/weighted-approval.yml index f72ada5942..e406d5ee2a 100644 --- a/.github/workflows/weighted-approval.yml +++ b/.github/workflows/weighted-approval.yml @@ -59,10 +59,11 @@ jobs: GITHUB_TOKEN: ${{ github.token }} GITHUB_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: node .github/review-ownership/check-approval.mjs - # A failed or cancelled run must not leave an earlier conclusion standing. A job cancelled - # after the decision was published keeps that decision instead of overwriting it with failure. + # A failed or cancelled run must not leave an earlier conclusion standing. The publish step + # records the conclusion it wrote, so a decision already published survives a later failure or + # cancellation, and a cancelled publish step that wrote nothing still concludes failure. - name: Publish approval setup failure - if: failure() || (cancelled() && steps.publish.outcome != 'success') + if: (failure() || cancelled()) && steps.publish.outputs.concluded == '' env: GITHUB_TOKEN: ${{ github.token }} GITHUB_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} diff --git a/scripts/ci-workflow.spec.ts b/scripts/ci-workflow.spec.ts index fc20391e2a..9c0ffc9d2d 100644 --- a/scripts/ci-workflow.spec.ts +++ b/scripts/ci-workflow.spec.ts @@ -1304,7 +1304,7 @@ describe('Weighted approval workflow', () => { expect(revokeIndex).toBeLessThan(setupIndex) expect(steps[revokeIndex]?.run).toBe('node .github/review-ownership/check-approval.mjs revoke') expect(steps.at(-1)).toMatchObject({ - if: "failure() || (cancelled() && steps.publish.outcome != 'success')", + if: "(failure() || cancelled()) && steps.publish.outputs.concluded == ''", run: 'node .github/review-ownership/check-approval.mjs failure', }) const pythonJob = workflowJob(loadWorkflow('.github/workflows/ci.yml'), 'python-sdk')