diff --git a/.github/workflows/automated-review-gate.yml b/.github/workflows/automated-review-gate.yml index 4b239b0516..634db0a391 100644 --- a/.github/workflows/automated-review-gate.yml +++ b/.github/workflows/automated-review-gate.yml @@ -24,8 +24,8 @@ jobs: target: name: resolve automated review target # Apply publisher eligibility before allocating a resolver runner. The - # resolver also gives source and merge-group publishers the same immutable - # source-SHA concurrency key. + # resolver also gives source and merge-group publishers the same per-pull + # concurrency key while keeping the immutable source SHA separate. if: >- github.event_name == 'merge_group' || ((github.event_name != 'issue_comment' || github.event.issue.pull_request) && @@ -39,8 +39,10 @@ jobs: timeout-minutes: 2 outputs: key: ${{ steps.resolve.outputs.key }} + head_sha: ${{ steps.resolve.outputs.head-sha }} pull_number: ${{ steps.resolve.outputs.pull-number }} status_id: ${{ steps.resolve.outputs.status-id }} + merge_group_status_id: ${{ steps.resolve.outputs.merge-group-status-id }} steps: - name: Resolve the current head commit id: resolve @@ -55,7 +57,7 @@ jobs: const match = /^(?:refs\/heads\/)?gh-readonly-queue\/.+\/pr-([1-9]\d*)-([0-9a-f]{40})$/i.exec(headRef); const pullNumber = match ? Number(match[1]) : undefined; return Number.isSafeInteger(pullNumber) - ? { pullNumber, sourceHeadSha: match[2].toLowerCase() } + ? { pullNumber, baseHeadSha: match[2].toLowerCase() } : undefined; }; const parseReviewWakeupIdentity = (run) => { @@ -102,6 +104,59 @@ jobs: core.setOutput("key", `run-${context.runId}`); return; } + // Publish the per-pull lock as soon as the serialized queue ref + // is trustworthy. Later resolver failures must serialize their + // emergency status with the normal merge-group publisher. + core.setOutput("key", `pr-${queueEntry.pullNumber}`); + const mergeGroupSha = context.payload.merge_group?.head_sha; + if (typeof mergeGroupSha !== "string" || !fullSha.test(mergeGroupSha)) { + core.setFailed("Could not resolve a valid merge group commit"); + return; + } + let mergeGroupStatusId = 0; + let mergeGroupStatusFound = false; + try { + for (let page = 1; page <= 5; page += 1) { + const response = await github.rest.repos.listCommitStatusesForRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: mergeGroupSha, + per_page: 100, + page, + }); + if (!Array.isArray(response.data)) { + throw new Error("Merge group status history is malformed"); + } + for (const status of response.data) { + if (status?.context !== "Automated review") continue; + if (!Number.isSafeInteger(status?.id) || status.id < 1) { + throw new Error("Merge group status identity is malformed"); + } + mergeGroupStatusId = status.id; + mergeGroupStatusFound = true; + break; + } + if (mergeGroupStatusFound || response.data.length < 100) break; + if (page === 5) { + throw new Error("Merge group status history exceeded 500 items"); + } + } + core.setOutput( + "merge-group-status-id", + String(mergeGroupStatusId), + ); + } catch (error) { + core.setFailed(`Could not capture the merge group status boundary: ${error}`); + return; + } + const baseSha = context.payload.merge_group?.base_sha; + if ( + typeof baseSha !== "string" || !fullSha.test(baseSha) || + queueEntry.baseHeadSha !== baseSha.toLowerCase() + ) { + core.setFailed("Merge queue base does not match its serialized lock"); + return; + } } const pullNumber = queueEntry?.pullNumber ?? context.payload.pull_request?.number ?? @@ -110,51 +165,50 @@ jobs: : wakeupSignal?.pullNumber); if (!Number.isSafeInteger(pullNumber) || pullNumber < 1) { core.setFailed("Could not resolve a valid pull request number"); - core.setOutput("key", `run-${context.runId}`); + if (!queueEntry) core.setOutput("key", `run-${context.runId}`); return; } - let headSha = queueEntry?.sourceHeadSha; - if (!headSha) { + let headSha; + try { + const response = await github.rest.git.getRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: `pull/${pullNumber}/head`, + }); + headSha = response.data?.object?.sha; + } catch (error) { try { - const response = await github.rest.git.getRef({ + const fallbackResponse = await github.rest.pulls.get({ owner: context.repo.owner, repo: context.repo.repo, - ref: `pull/${pullNumber}/head`, + pull_number: pullNumber, }); - headSha = response.data?.object?.sha; - } catch (error) { - try { - const fallbackResponse = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: pullNumber, - }); - const fallbackHead = fallbackResponse.data?.head?.sha; - if (typeof fallbackHead === "string" && fullSha.test(fallbackHead)) { - headSha = fallbackHead; - } - } catch (fallbackError) { - core.error( - `Could not resolve a failure target from the pull request: ${fallbackError}`, - ); - } - const eventFailureHead = - context.payload.pull_request?.number === pullNumber - ? context.payload.pull_request?.head?.sha - : undefined; - if (!headSha && fullSha.test(eventFailureHead ?? "")) { - headSha = eventFailureHead; + const fallbackHead = fallbackResponse.data?.head?.sha; + if (typeof fallbackHead === "string" && fullSha.test(fallbackHead)) { + headSha = fallbackHead; } - core.setFailed(`Could not resolve the pull request head ref: ${error}`); + } catch (fallbackError) { + core.error( + `Could not resolve a failure target from the pull request: ${fallbackError}`, + ); } + const eventFailureHead = + context.payload.pull_request?.number === pullNumber + ? context.payload.pull_request?.head?.sha + : undefined; + if (!headSha && fullSha.test(eventFailureHead ?? "")) { + headSha = eventFailureHead; + } + core.setFailed(`Could not resolve the pull request head ref: ${error}`); } if (!fullSha.test(headSha ?? "")) { core.setFailed("Could not resolve a valid review target commit"); - core.setOutput("key", `run-${context.runId}`); + if (!queueEntry) core.setOutput("key", `run-${context.runId}`); return; } headSha = headSha.toLowerCase(); - core.setOutput("key", headSha); + core.setOutput("key", `pr-${pullNumber}`); + core.setOutput("head-sha", headSha); core.setOutput("pull-number", String(pullNumber)); if (wakeupSignal) { let pullRequest; @@ -243,7 +297,17 @@ jobs: (needs.target.result != 'success' || needs.merge_group.result != 'success') permissions: + contents: read statuses: write + env: + TARGET_RESULT: ${{ needs.target.result }} + TARGET_STATUS_ID: ${{ needs.target.outputs.merge_group_status_id }} + PUBLISHER_STATUS_ID: ${{ needs.merge_group.outputs.status_id }} + concurrency: + # Parsed merge-group runs share the normal per-pull lock. The raw-ref + # fallback only applies when target resolution could not parse the ref. + group: automated-review-${{ needs.target.outputs.key || format('merge-group-{0}', github.event.merge_group.head_ref) }} + queue: max runs-on: ubuntu-latest timeout-minutes: 2 steps: @@ -257,6 +321,124 @@ jobs: core.setFailed("Could not resolve the merge group failure target"); return; } + const headRef = context.payload.merge_group?.head_ref; + const pullMatch = typeof headRef === "string" + ? /^(?:refs\/heads\/)?gh-readonly-queue\/.+\/pr-([1-9]\d*)-[0-9a-f]{40}$/i.exec(headRef) + : undefined; + const pullNumber = pullMatch ? Number(pullMatch[1]) : undefined; + const parseStatusId = (value) => { + if (typeof value !== "string" || value === "") return undefined; + const statusId = Number(value); + return Number.isSafeInteger(statusId) && statusId >= 0 + ? statusId + : undefined; + }; + let selectMergeGroupFailureStatusBoundary = ({ + targetResult, + targetStatusId, + publisherStatusId, + }) => { + const statusId = targetResult === "success" && + publisherStatusId !== undefined + ? publisherStatusId + : targetStatusId; + return Number.isSafeInteger(statusId) && statusId >= 0 + ? statusId + : undefined; + }; + let shouldPreserveLaterMergeGroupSuccess = ({ + latestStatus, + reconciliationStatusId, + pullNumber, + }) => + Number.isSafeInteger(reconciliationStatusId) && + reconciliationStatusId >= 0 && + Number.isSafeInteger(pullNumber) && pullNumber >= 1 && + Number.isSafeInteger(latestStatus?.id) && + latestStatus.id !== reconciliationStatusId && + latestStatus?.context === "Automated review" && + latestStatus?.state === "success" && + latestStatus?.creator?.login === "github-actions[bot]" && + latestStatus?.creator?.id === 41898282 && + latestStatus?.creator?.type === "Bot" && + latestStatus?.description === + `Reused exact-head review for PR #${pullNumber}`; + try { + const response = await github.rest.repos.getContent({ + owner: context.repo.owner, + repo: context.repo.repo, + path: "scripts/ci/automated-review-gate.mjs", + ref: context.payload.repository.default_branch, + }); + const file = response.data; + if ( + Array.isArray(file) || file?.type !== "file" || + file?.encoding !== "base64" || + typeof file?.content !== "string" + ) throw new Error("Trusted review gate content is malformed"); + const gateUrl = `data:text/javascript;base64,${ + Buffer.from(file.content, "base64").toString("base64") + }`; + const gate = await import(gateUrl); + if ( + typeof gate.selectMergeGroupFailureStatusBoundary !== "function" || + typeof gate.shouldPreserveLaterMergeGroupSuccess !== "function" + ) { + throw new Error("Trusted review boundary helper is unavailable"); + } + selectMergeGroupFailureStatusBoundary = + gate.selectMergeGroupFailureStatusBoundary; + shouldPreserveLaterMergeGroupSuccess = + gate.shouldPreserveLaterMergeGroupSuccess; + } catch (error) { + // This job must still close the gate when the trusted helper is + // the component that failed. Keep an independent pinned check. + core.warning(`Using the independent boundary check: ${error}`); + } + const reconciliationStatusId = selectMergeGroupFailureStatusBoundary({ + targetResult: process.env.TARGET_RESULT, + targetStatusId: parseStatusId(process.env.TARGET_STATUS_ID), + publisherStatusId: parseStatusId(process.env.PUBLISHER_STATUS_ID), + }); + if (reconciliationStatusId === undefined) { + core.warning("The merge group status boundary is unavailable"); + } + let latestStatus; + try { + for (let page = 1; page <= 5; page += 1) { + const response = await github.rest.repos.listCommitStatusesForRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: mergeGroupSha, + per_page: 100, + page, + }); + if (!Array.isArray(response.data)) { + throw new Error("Merge group status history is malformed"); + } + latestStatus = response.data.find((status) => + status?.context === "Automated review" + ); + if (latestStatus || response.data.length < 100) break; + if (page === 5) { + throw new Error("Merge group status history exceeded 500 items"); + } + } + } catch (error) { + // If the optional stale-run check is unavailable, closing the + // required status remains the fail-closed result. + core.warning(`Could not check for a later merge group publisher: ${error}`); + } + if (shouldPreserveLaterMergeGroupSuccess({ + latestStatus, + reconciliationStatusId, + pullNumber, + })) { + core.notice( + `Preserved merge group success ${latestStatus.id} published after boundary ${reconciliationStatusId}.`, + ); + return; + } await github.rest.repos.createCommitStatus({ owner: context.repo.owner, repo: context.repo.repo, @@ -285,9 +467,12 @@ jobs: # executes pull request code. needs: target if: github.event_name != 'merge_group' + outputs: + force_invalidate: ${{ steps.publish.outputs.force-invalidate }} + source_status_id: ${{ steps.publish.outputs.source-status-id }} concurrency: - # Every pull request event for one head must serialize on the same key so - # an older pending write cannot win. + # Every event for one pull request must serialize across force-pushes so + # an old-head failure cannot close the current merge-group gate. group: automated-review-${{ needs.target.outputs.key }} queue: max runs-on: ubuntu-latest @@ -297,7 +482,7 @@ jobs: id: publish uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: - TARGET_SHA: ${{ needs.target.outputs.key }} + TARGET_SHA: ${{ needs.target.outputs.head_sha }} PULL_NUMBER: ${{ needs.target.outputs.pull_number }} with: retries: 3 @@ -368,6 +553,28 @@ jobs: ? `base-${context.runId}` : undefined, }); + if (Number.isSafeInteger(result.statusId)) { + core.setOutput("source-status-id", String(result.statusId)); + } + if ( + result.state === "success" && + (!Number.isSafeInteger(result.statusId) || result.statusId < 1) + ) { + core.setFailed( + "Automated review success did not return a source status identity.", + ); + return "failure"; + } + const forceInvalidateCurrentSource = () => { + if (!Number.isSafeInteger(result.statusId) || result.statusId < 1) { + core.setFailed( + "Cannot force invalidation without the source status identity.", + ); + return false; + } + core.setOutput("force-invalidate", "true"); + return true; + }; let queueResultCount; if ( result.state === "failure" || @@ -382,14 +589,37 @@ jobs: }); queueResultCount = queueFailures; } else { - const queueResults = await reconcileActiveMergeGroupReviewStatuses({ - github, - owner: context.repo.owner, - repo: context.repo.repo, - pullNumber, - sourceHeadSha: headSha, - baseRef: result.baseRef, - }); + let queueResults; + try { + queueResults = await reconcileActiveMergeGroupReviewStatuses({ + github, + owner: context.repo.owner, + repo: context.repo.repo, + pullNumber, + sourceHeadSha: headSha, + baseRef: result.baseRef, + }); + } catch (error) { + // The source success above belongs to this failed run. Tell + // fallback invalidation not to treat it as a later repair. + if (!forceInvalidateCurrentSource()) return "failure"; + throw error; + } + // A failure that never reached its synthetic commit leaves the + // queue's prior success as live review proof. The trusted gate + // throws on that, but the loaded module comes from the default + // branch, so verify the collected results here as well and fail + // the run: the fallback invalidation job keys on this failure. + const unreplaced = queueResults.filter((entry) => + entry?.state === "failure" && entry?.published !== true + ).length; + if (unreplaced > 0) { + if (!forceInvalidateCurrentSource()) return "failure"; + core.setFailed( + `Review proof was not replaced on ${unreplaced} active merge queue commit(s).`, + ); + return "failure"; + } queueResultCount = queueResults.length; } if (queueResultCount > 0) { @@ -427,7 +657,7 @@ jobs: steps.publish.outputs.result == 'pending' uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: - TARGET_SHA: ${{ needs.target.outputs.key }} + TARGET_SHA: ${{ needs.target.outputs.head_sha }} with: retries: 3 script: | @@ -487,12 +717,14 @@ jobs: pull-requests: read statuses: write env: - TARGET_SHA: ${{ needs.target.outputs.key }} + TARGET_SHA: ${{ needs.target.outputs.head_sha }} PULL_NUMBER: ${{ needs.target.outputs.pull_number }} RECONCILIATION_STATUS_ID: ${{ needs.target.outputs.status_id }} + SOURCE_STATUS_ID: ${{ needs.review.outputs.source_status_id }} + FORCE_INVALIDATE: ${{ needs.review.outputs.force_invalidate }} concurrency: - # Share the source-head publisher lock so fallback failures cannot race - # successful source or merge-group status writes. + # Share the per-pull publisher lock so fallback failures cannot race + # successful source or merge-group status writes across force-pushes. group: automated-review-${{ needs.target.outputs.key }} queue: max runs-on: ubuntu-latest @@ -506,7 +738,15 @@ jobs: const fullSha = /^[0-9a-f]{40}$/i; const pullNumber = Number(process.env.PULL_NUMBER); const headSha = process.env.TARGET_SHA; - const statusIdText = process.env.RECONCILIATION_STATUS_ID ?? ""; + const forceInvalidateText = process.env.FORCE_INVALIDATE ?? ""; + if (forceInvalidateText !== "" && forceInvalidateText !== "true") { + core.setFailed("The forced invalidation signal is malformed"); + return; + } + const forceInvalidate = forceInvalidateText === "true"; + const statusIdText = forceInvalidate + ? process.env.SOURCE_STATUS_ID ?? "" + : process.env.RECONCILIATION_STATUS_ID ?? ""; const reconciliationStatusId = statusIdText === "" ? undefined : Number(statusIdText); @@ -528,6 +768,14 @@ jobs: core.setFailed("The review status boundary is invalid"); return; } + const parseMergeQueuePullNumber = (headRef) => { + if (typeof headRef !== "string") return undefined; + const match = /^(?:refs\/heads\/)?gh-readonly-queue\/.+\/pr-([1-9]\d*)-([0-9a-f]{40})$/i.exec(headRef); + const number = match ? Number(match[1]) : undefined; + return Number.isSafeInteger(number) + ? { pullNumber: number, baseHeadSha: match[2].toLowerCase() } + : undefined; + }; const collectAll = async (endpoint, parameters, source) => { const items = []; for await ( @@ -546,13 +794,35 @@ jobs: } return items; }; + const resolveQueueRefTarget = async (ref) => { + let response; + try { + response = await github.rest.git.getRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref, + }); + } catch (error) { + if (error?.status === 404) return undefined; + throw error; + } + const sha = response?.data?.object?.sha; + if (!fullSha.test(sha ?? "")) { + throw new Error("Merge queue ref response has a malformed commit"); + } + return sha; + }; const publishIndependentFailure = async () => { const pull = await github.rest.pulls.get({ owner: context.repo.owner, repo: context.repo.repo, pull_number: pullNumber, }); - if (pull.data?.head?.sha?.toLowerCase() !== headSha.toLowerCase()) { + const pullHeadSha = pull.data?.head?.sha; + if (!fullSha.test(pullHeadSha ?? "")) { + throw new Error("Pull request head response has a malformed commit"); + } + if (pullHeadSha.toLowerCase() !== headSha.toLowerCase()) { return { headSha, queueFailures: 0, skipped: true }; } const baseRepositoryId = pull.data?.base?.repo?.id; @@ -611,21 +881,98 @@ jobs: { owner: context.repo.owner, repo: context.repo.repo, - ref: `heads/gh-readonly-queue/${baseRef}/pr-${pullNumber}-${headSha}`, + ref: "heads/gh-readonly-queue/", }, "merge queue refs", ); const seen = new Set(); - const expectedQueueRef = - `refs/heads/gh-readonly-queue/${baseRef}/pr-${pullNumber}-${headSha}`; for (const queueRef of refs) { - if (queueRef?.ref !== expectedQueueRef) continue; + const parsed = parseMergeQueuePullNumber(queueRef?.ref); + if (parsed?.pullNumber !== pullNumber) continue; const mergeGroupSha = queueRef?.object?.sha; if (!fullSha.test(mergeGroupSha ?? "")) { throw new Error("Merge queue ref has a malformed commit"); } if (seen.has(mergeGroupSha.toLowerCase())) continue; - seen.add(mergeGroupSha.toLowerCase()); + const latestPull = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pullNumber, + }); + const latestHeadSha = latestPull.data?.head?.sha; + if (!fullSha.test(latestHeadSha ?? "")) { + throw new Error("Pull request head response has a malformed commit"); + } + if ( + latestHeadSha.toLowerCase() !== headSha.toLowerCase() + ) continue; + let targetVerified = false; + try { + const bindingResponse = await github.graphql( + `query ActiveMergeQueueBinding($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + number + state + headRefOid + baseRefName + mergeQueueEntry { + baseCommit { oid } + headCommit { oid } + } + } + } + }`, + { + owner: context.repo.owner, + repo: context.repo.repo, + number: pullNumber, + }, + ); + const boundPull = bindingResponse?.repository?.pullRequest; + const boundEntry = boundPull?.mergeQueueEntry; + const activeBindingMismatch = + boundPull?.number !== pullNumber || + boundPull?.state !== "OPEN" || + boundPull?.headRefOid?.toLowerCase() !== headSha.toLowerCase() || + boundPull?.baseRefName !== baseRef || + boundEntry?.baseCommit?.oid?.toLowerCase() !== + parsed.baseHeadSha || + boundEntry?.headCommit?.oid?.toLowerCase() !== + mergeGroupSha.toLowerCase(); + if (!activeBindingMismatch) { + const liveRef = await github.rest.git.getRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: queueRef.ref.slice("refs/".length), + }); + targetVerified = + liveRef?.data?.object?.sha?.toLowerCase() === + mergeGroupSha.toLowerCase(); + } + } catch { + targetVerified = false; + } + if (!targetVerified) { + const liveTarget = await resolveQueueRefTarget( + queueRef.ref.slice("refs/".length), + ); + targetVerified = + liveTarget?.toLowerCase() === mergeGroupSha.toLowerCase(); + } + if (!targetVerified) continue; + const finalPull = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pullNumber, + }); + const finalHeadSha = finalPull.data?.head?.sha; + if (!fullSha.test(finalHeadSha ?? "")) { + throw new Error("Pull request head response has a malformed commit"); + } + if ( + finalHeadSha.toLowerCase() !== headSha.toLowerCase() + ) continue; await github.rest.repos.createCommitStatus({ owner: context.repo.owner, repo: context.repo.repo, @@ -635,6 +982,7 @@ jobs: description: `Could not revalidate review for PR #${pullNumber}`, target_url: pullUrl, }); + seen.add(mergeGroupSha.toLowerCase()); } return { headSha, @@ -686,6 +1034,8 @@ jobs: name: reuse exact-head review for merge group if: github.event_name == 'merge_group' needs: target + outputs: + status_id: ${{ steps.reuse.outputs.merge-group-status-id }} permissions: contents: read pull-requests: read @@ -697,13 +1047,56 @@ jobs: timeout-minutes: 5 steps: - name: Reuse the source pull request review + id: reuse uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: - SOURCE_HEAD_SHA: ${{ needs.target.outputs.key }} + SOURCE_HEAD_SHA: ${{ needs.target.outputs.head_sha }} PULL_NUMBER: ${{ needs.target.outputs.pull_number }} with: retries: 3 script: | + const fullSha = /^[0-9a-f]{40}$/i; + const mergeGroupSha = context.payload.merge_group?.head_sha; + if (!fullSha.test(mergeGroupSha ?? "")) { + core.setFailed("Could not resolve a valid merge group commit"); + return; + } + let mergeGroupStatusId = 0; + let mergeGroupStatusFound = false; + try { + for (let page = 1; page <= 5; page += 1) { + const response = await github.rest.repos.listCommitStatusesForRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: mergeGroupSha, + per_page: 100, + page, + }); + if (!Array.isArray(response.data)) { + throw new Error("Merge group status history is malformed"); + } + for (const status of response.data) { + if (status?.context !== "Automated review") continue; + if (!Number.isSafeInteger(status?.id) || status.id < 1) { + throw new Error("Merge group status identity is malformed"); + } + mergeGroupStatusId = status.id; + mergeGroupStatusFound = true; + break; + } + if (mergeGroupStatusFound || response.data.length < 100) break; + if (page === 5) { + throw new Error("Merge group status history exceeded 500 items"); + } + } + core.setOutput( + "merge-group-status-id", + String(mergeGroupStatusId), + ); + } catch (error) { + core.setFailed(`Could not capture the publisher status boundary: ${error}`); + return; + } let parseMergeQueuePullNumber; let publishMergeGroupReviewStatus; try { @@ -739,12 +1132,74 @@ jobs: } const pullNumber = Number(process.env.PULL_NUMBER); const sourceHeadSha = process.env.SOURCE_HEAD_SHA; + const baseSha = context.payload.merge_group.base_sha; + // The trusted default-branch helper returns sourceHeadSha until + // this compatibility fix itself has passed the merge queue. + const serializedBaseSha = queueEntry.baseHeadSha ?? + queueEntry.sourceHeadSha; if ( !Number.isSafeInteger(pullNumber) || pullNumber !== queueEntry.pullNumber || - sourceHeadSha !== queueEntry.sourceHeadSha + !fullSha.test(sourceHeadSha ?? "") || + !fullSha.test(baseSha ?? "") || + !fullSha.test(serializedBaseSha ?? "") || + baseSha.toLowerCase() !== serializedBaseSha ) { - core.setFailed("Merge queue source does not match its serialized lock"); + core.setFailed("Merge queue identity does not match its serialized lock"); + return; + } + const bindingResponse = await github.graphql( + `query ActiveMergeQueueBinding($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + number + state + headRefOid + baseRefName + mergeQueueEntry { + baseCommit { oid } + headCommit { oid } + } + } + } + }`, + { + owner: context.repo.owner, + repo: context.repo.repo, + number: pullNumber, + }, + ); + const boundPull = bindingResponse?.repository?.pullRequest; + const boundEntry = boundPull?.mergeQueueEntry; + const baseRef = boundPull?.baseRefName; + const eventRef = context.payload.merge_group.head_ref; + const normalizedEventRef = typeof eventRef === "string" && + eventRef.startsWith("refs/heads/") + ? eventRef + : `refs/heads/${eventRef}`; + const expectedRef = typeof baseRef === "string" + ? `refs/heads/gh-readonly-queue/${baseRef}/pr-${pullNumber}-${serializedBaseSha}` + : ""; + if ( + boundPull?.number !== pullNumber || boundPull?.state !== "OPEN" || + !fullSha.test(mergeGroupSha ?? "") || + boundPull?.headRefOid?.toLowerCase() !== sourceHeadSha || + boundEntry?.baseCommit?.oid?.toLowerCase() !== serializedBaseSha || + boundEntry?.headCommit?.oid?.toLowerCase() !== mergeGroupSha || + typeof baseRef !== "string" || baseRef.length === 0 || + baseRef.length > 1024 || baseRef.includes("\0") || + normalizedEventRef !== expectedRef + ) { + core.setFailed("Merge group is not bound to the current pull request head"); + return; + } + const liveRef = await github.rest.git.getRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: expectedRef.slice("refs/".length), + }); + if (liveRef?.data?.object?.sha?.toLowerCase() !== mergeGroupSha) { + core.setFailed("Merge queue ref no longer targets this merge group"); return; } const result = await publishMergeGroupReviewStatus({ @@ -753,7 +1208,8 @@ jobs: repo: context.repo.repo, pullNumber, sourceHeadSha, - mergeGroupSha: context.payload.merge_group.head_sha, + baseHeadSha: serializedBaseSha, + mergeGroupSha, }); if (result.state === "success") { core.notice( diff --git a/scripts/ci/automated-review-gate.mjs b/scripts/ci/automated-review-gate.mjs index 579403aa36..7b49a3ce84 100644 --- a/scripts/ci/automated-review-gate.mjs +++ b/scripts/ci/automated-review-gate.mjs @@ -23,6 +23,10 @@ export const AUTOMATED_REVIEW_STATUS_CONTEXT = "Automated review"; const REVIEW_WAKEUP_PATH = ".github/workflows/automated-review-wakeup.yml"; +function isPositiveStatusId(value) { + return Number.isSafeInteger(value) && value > 0; +} + /** Produce a compact, non-ambiguous binding for a pull request base. */ export function reviewBaseBinding(baseRepositoryId, baseRef) { if (!Number.isSafeInteger(baseRepositoryId) || baseRepositoryId < 1) { @@ -222,16 +226,13 @@ function evidenceFreshness( timelineEvent, boundary, timeline, + timeMode = "created", ) { if (boundary === undefined) return "newer"; - const evidenceTime = Date.parse( - timelineEvent === "reviewed" - ? evidence?.submitted_at ?? "" - : evidence?.created_at ?? "", - ); - if (!Number.isFinite(evidenceTime)) return "ambiguous"; - if (evidenceTime < boundary.time) return "older"; - if (evidenceTime > boundary.time) return "newer"; + const time = evidenceTime(evidence, timelineEvent, timeMode); + if (!Number.isFinite(time)) return "ambiguous"; + if (time < boundary.time) return "older"; + if (time > boundary.time) return "newer"; if (boundary.timelineEvent === undefined) return "ambiguous"; const boundaryPosition = timelinePosition( timeline, @@ -249,6 +250,64 @@ function evidenceFreshness( return evidencePosition > boundaryPosition ? "newer" : "older"; } +function evidenceTime(evidence, timelineEvent, timeMode = "created") { + if (timelineEvent === "reviewed") { + return Date.parse(evidence?.submitted_at ?? ""); + } + const createdAt = Date.parse(evidence?.created_at ?? ""); + if (timeMode !== "finding") return createdAt; + const updatedAt = Date.parse(evidence?.updated_at ?? ""); + if (Number.isFinite(createdAt) && Number.isFinite(updatedAt)) { + return Math.max(createdAt, updatedAt); + } + return Number.isFinite(updatedAt) ? updatedAt : createdAt; +} + +function isProvablyUneditedComment(comment) { + const createdAt = Date.parse(comment?.created_at ?? ""); + const updatedAt = Date.parse(comment?.updated_at ?? ""); + return Number.isFinite(createdAt) && Number.isFinite(updatedAt) && + updatedAt === createdAt; +} + +function isEditedComment(comment) { + const createdAt = Date.parse(comment?.created_at ?? ""); + const updatedAt = Date.parse(comment?.updated_at ?? ""); + return Number.isFinite(createdAt) && Number.isFinite(updatedAt) && + updatedAt !== createdAt; +} + +function isEvidenceProvablyLater(candidate, current, timeline) { + const candidateTime = candidate.time ?? evidenceTime( + candidate.evidence, + candidate.timelineEvent, + ); + const currentTime = current.time ?? + evidenceTime(current.evidence, current.timelineEvent); + if (!Number.isFinite(candidateTime) || !Number.isFinite(currentTime)) { + return false; + } + if (candidateTime !== currentTime) return candidateTime > currentTime; + // The timeline records a comment's creation position, not its later edit. + // It therefore cannot order a same-second success against an edited finding. + if ( + current.timelineEvent === "commented" && + isEditedComment(current.evidence) + ) return false; + const candidatePosition = timelinePosition( + timeline, + candidate.timelineEvent, + candidate.evidence?.id, + ); + const currentPosition = timelinePosition( + timeline, + current.timelineEvent, + current.evidence?.id, + ); + return candidatePosition !== undefined && currentPosition !== undefined && + candidatePosition > currentPosition; +} + function reviewResetDescription(pullNumber, baseBinding, requestKey) { const prefix = `PR#${pullNumber} reset base:${baseBinding}`; return requestKey === undefined @@ -315,8 +374,8 @@ export async function findAutomatedReview( validReviewNotBefore, latestBaseRefChange(events), ); - let codexApproval; - let codexReviewFinding = false; + const codexSuccesses = []; + const codexFindings = []; { const latestHumanReviews = new Map(); for (const [index, review] of reviews.entries()) { @@ -336,21 +395,29 @@ export async function findAutomatedReview( freshness === "newer" && state === "APPROVED" && isPinnedBot(review?.user, CODEX_LOGIN) ) { - codexApproval = { - reviewer: CODEX_LOGIN, - source: "pull-request-review", - state, - url: typeof review.html_url === "string" - ? review.html_url - : undefined, - }; + codexSuccesses.push({ + evidence: review, + timelineEvent: "reviewed", + proof: { + reviewer: CODEX_LOGIN, + source: "pull-request-review", + state, + url: typeof review.html_url === "string" + ? review.html_url + : undefined, + }, + }); continue; } if ( freshness !== "older" && isPinnedBot(review?.user, CODEX_LOGIN) && (state === "COMMENTED" || state === "CHANGES_REQUESTED") ) { - codexReviewFinding = true; + codexFindings.push({ + evidence: review, + freshness, + timelineEvent: "reviewed", + }); continue; } if ( @@ -381,20 +448,11 @@ export async function findAutomatedReview( } } - let codexNoFindings; - let codexFinding = false; for (const comment of comments) { if ( !isPinnedBot(comment?.user, CODEX_LOGIN) || typeof comment?.body !== "string" ) continue; - const freshness = evidenceFreshness( - comment, - "commented", - boundary, - timeline, - ); - if (freshness === "older") continue; const reviewedCommits = [...comment.body.matchAll( new RegExp(CODEX_REVIEWED_COMMIT, "gi"), )]; @@ -410,25 +468,49 @@ export async function findAutomatedReview( typeof resolved === "string" && FULL_SHA.test(resolved) && resolved.toLowerCase() === headSha.toLowerCase() ) { + const cleanVerdict = comment.body.startsWith(CODEX_NO_FINDINGS); + const freshness = evidenceFreshness( + comment, + "commented", + boundary, + timeline, + cleanVerdict ? "created" : "finding", + ); + if (freshness === "older") continue; if ( - freshness === "newer" && comment.body.startsWith(CODEX_NO_FINDINGS) + freshness === "newer" && cleanVerdict && + isProvablyUneditedComment(comment) ) { - codexNoFindings = { - reviewer: CODEX_LOGIN, - source: "codex-comment", - state: "COMMENTED", - url: typeof comment.html_url === "string" - ? comment.html_url - : undefined, - }; - } else { - codexFinding = true; + codexSuccesses.push({ + evidence: comment, + time: evidenceTime(comment, "commented", "created"), + timelineEvent: "commented", + proof: { + reviewer: CODEX_LOGIN, + source: "codex-comment", + state: "COMMENTED", + url: typeof comment.html_url === "string" + ? comment.html_url + : undefined, + }, + }); + } else if (!cleanVerdict) { + codexFindings.push({ + evidence: comment, + freshness, + time: evidenceTime(comment, "commented", "finding"), + timelineEvent: "commented", + }); } } } - return codexReviewFinding || codexFinding - ? undefined - : codexApproval ?? codexNoFindings; + if (codexFindings.length === 0) return codexSuccesses[0]?.proof; + return codexSuccesses.find((success) => + codexFindings.every((finding) => + finding.freshness === "newer" && + isEvidenceProvablyLater(success, finding, timeline) + ) + )?.proof; } async function collectAll(github, endpoint, parameters, source) { @@ -453,7 +535,11 @@ async function collectAll(github, endpoint, parameters, source) { async function resolveCommitRef(github, common, ref) { try { const response = await github.rest.repos.getCommit({ ...common, ref }); - return response?.data?.sha; + const sha = response?.data?.sha; + if (!FULL_SHA.test(sha ?? "")) { + throw new Error("Commit ref response has a malformed commit"); + } + return sha; } catch (error) { if ( typeof error === "object" && error !== null && error.status === 404 @@ -462,6 +548,116 @@ async function resolveCommitRef(github, common, ref) { } } +async function resolveQueueRefTarget(github, common, ref) { + let response; + try { + response = await github.rest.git.getRef({ ...common, ref }); + } catch (error) { + if ( + typeof error === "object" && error !== null && error.status === 404 + ) return undefined; + throw error; + } + const sha = response?.data?.object?.sha; + if (!FULL_SHA.test(sha ?? "")) { + throw new Error("Merge queue ref response has a malformed commit"); + } + return sha; +} + +function mergeQueueTarget(queueRef, pullNumber) { + const parsed = parseMergeQueuePullNumber(queueRef?.ref); + if (parsed?.pullNumber !== pullNumber) return undefined; + const mergeGroupSha = queueRef?.object?.sha; + if (!FULL_SHA.test(mergeGroupSha ?? "")) { + throw new Error("Merge queue ref has a malformed commit"); + } + return { + baseHeadSha: parsed.baseHeadSha, + mergeGroupSha, + key: mergeGroupSha.toLowerCase(), + ref: queueRef.ref.startsWith("refs/") + ? queueRef.ref.slice("refs/".length) + : queueRef.ref, + }; +} + +async function queueTargetIsActive({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + target, +}) { + try { + await requireActiveMergeQueueBinding({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + baseHeadSha: target.baseHeadSha, + mergeGroupSha: target.mergeGroupSha, + }); + return true; + } catch { + const liveTarget = await resolveQueueRefTarget( + github, + { owner, repo }, + target.ref, + ); + return liveTarget?.toLowerCase() === target.key; + } +} + +async function publishQueueReviewResolutionFailure({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + pullUrl, + queueRef, + seen, +}) { + const target = mergeQueueTarget(queueRef, pullNumber); + if (!target || seen.has(target.key)) return; + const common = { owner, repo }; + const currentHeadSha = await resolveCommitRef( + github, + common, + `refs/pull/${pullNumber}/head`, + ); + if (currentHeadSha?.toLowerCase() !== sourceHeadSha.toLowerCase()) return; + if ( + !await queueTargetIsActive({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + target, + }) + ) return; + const finalSourceHeadSha = await resolveCommitRef( + github, + common, + `refs/pull/${pullNumber}/head`, + ); + if (finalSourceHeadSha?.toLowerCase() !== sourceHeadSha.toLowerCase()) return; + await github.rest.repos.createCommitStatus({ + owner, + repo, + sha: target.mergeGroupSha, + state: "failure", + context: AUTOMATED_REVIEW_STATUS_CONTEXT, + description: `Could not revalidate review for PR #${pullNumber}`, + target_url: pullUrl, + }); + seen.add(target.key); +} + /** Fail one source head and every active queue commit derived from it. */ export async function publishReviewResolutionFailure({ github, @@ -477,6 +673,14 @@ export async function publishReviewResolutionFailure({ if (!FULL_SHA.test(sourceHeadSha)) { throw new Error("Pull request source commit is malformed"); } + const currentHeadSha = await resolveCommitRef( + github, + { owner, repo }, + `refs/pull/${pullNumber}/head`, + ); + if (currentHeadSha?.toLowerCase() !== sourceHeadSha.toLowerCase()) { + return { queueFailures: 0, skipped: true }; + } await github.rest.repos.createCommitStatus({ owner, repo, @@ -494,28 +698,18 @@ export async function publishReviewResolutionFailure({ ); const seen = new Set(); for (const queueRef of refs) { - const parsed = parseMergeQueuePullNumber(queueRef?.ref); - const mergeGroupSha = queueRef?.object?.sha; - if ( - parsed?.pullNumber !== pullNumber || - parsed?.sourceHeadSha !== sourceHeadSha.toLowerCase() - ) continue; - if (!FULL_SHA.test(mergeGroupSha ?? "")) { - throw new Error("Merge queue ref has a malformed commit"); - } - if (seen.has(mergeGroupSha.toLowerCase())) continue; - seen.add(mergeGroupSha.toLowerCase()); - await github.rest.repos.createCommitStatus({ + await publishQueueReviewResolutionFailure({ + github, owner, repo, - sha: mergeGroupSha, - state: "failure", - context: AUTOMATED_REVIEW_STATUS_CONTEXT, - description: `Could not revalidate review for PR #${pullNumber}`, - target_url: pullUrl, + pullNumber, + sourceHeadSha, + pullUrl, + queueRef, + seen, }); } - return { queueFailures: seen.size }; + return { queueFailures: seen.size, skipped: false }; } async function isCurrentlyTrustedHuman( @@ -697,7 +891,7 @@ export async function publishAutomatedReviewStatus({ else { description = `PR#${pullNumber} waits for review ${headSha.slice(0, 12)}`; } - await github.rest.repos.createCommitStatus({ + const statusResponse = await github.rest.repos.createCommitStatus({ owner, repo, sha: headSha, @@ -706,7 +900,30 @@ export async function publishAutomatedReviewStatus({ description, target_url: review?.url ?? pullUrl, }); - return { state, review, failure, description, baseRef }; + let statusId = statusResponse?.data?.id; + if (state === "success" && !isPositiveStatusId(statusId)) { + failure = new Error("Published review status identity is malformed"); + state = "failure"; + review = undefined; + description = `PR#${pullNumber} review status unavailable`; + const failureResponse = await github.rest.repos.createCommitStatus({ + owner, + repo, + sha: headSha, + state, + context: AUTOMATED_REVIEW_STATUS_CONTEXT, + description, + target_url: pullUrl, + }); + statusId = failureResponse?.data?.id; + if (!isPositiveStatusId(statusId)) { + throw failure; + } + } + if (statusId !== undefined && !isPositiveStatusId(statusId)) { + throw new Error("Published review status identity is malformed"); + } + return { state, review, failure, description, baseRef, statusId }; } /** @@ -798,24 +1015,63 @@ export async function invalidateReviewProof({ ? response.data.html_url : undefined, }); - return { headSha, description, ...result, skipped: false }; + return { headSha, description, ...result }; } -/** Extract the pull request represented by a merge queue head ref. */ +/** Extract the pull request and base commit represented by a merge queue ref. */ export function parseMergeQueuePullNumber(headRef) { if (typeof headRef !== "string") return undefined; const match = - /^(?:refs\/heads\/)?gh-readonly-queue\/.+\/pr-([1-9]\d*)-([0-9a-f]{40})$/i.exec( - headRef, - ); + /^(?:refs\/heads\/)?gh-readonly-queue\/.+\/pr-([1-9]\d*)-([0-9a-f]{40})$/i + .exec( + headRef, + ); if (!match) return undefined; const pullNumber = Number(match[1]); return Number.isSafeInteger(pullNumber) - ? { pullNumber, sourceHeadSha: match[2].toLowerCase() } + ? { pullNumber, baseHeadSha: match[2].toLowerCase() } + : undefined; +} + +/** Select the boundary captured by the publisher that actually failed. */ +export function selectMergeGroupFailureStatusBoundary({ + targetResult, + targetStatusId, + publisherStatusId, +}) { + const statusId = targetResult === "success" && + publisherStatusId !== undefined + ? publisherStatusId + : targetStatusId; + return Number.isSafeInteger(statusId) && statusId >= 0 + ? statusId : undefined; } -function assertMergeGroupInputs({ pullNumber, sourceHeadSha, mergeGroupSha }) { +/** Preserve a trusted merge-group success written after one run's boundary. */ +export function shouldPreserveLaterMergeGroupSuccess({ + latestStatus, + reconciliationStatusId, + pullNumber, +}) { + return Number.isSafeInteger(reconciliationStatusId) && + reconciliationStatusId >= 0 && + Number.isSafeInteger(pullNumber) && pullNumber >= 1 && + Number.isSafeInteger(latestStatus?.id) && + latestStatus.id !== reconciliationStatusId && + latestStatus?.context === AUTOMATED_REVIEW_STATUS_CONTEXT && + latestStatus?.state === "success" && + isPinnedBot(latestStatus?.creator, GITHUB_ACTIONS_LOGIN) && + latestStatus?.description === + `Reused exact-head review for PR #${pullNumber}`; +} + +function assertMergeGroupInputs({ + pullNumber, + sourceHeadSha, + baseHeadSha, + mergeGroupSha, +}) { if (!Number.isSafeInteger(pullNumber) || pullNumber < 1) { throw new Error("Merge queue pull request number is invalid"); } @@ -825,6 +1081,9 @@ function assertMergeGroupInputs({ pullNumber, sourceHeadSha, mergeGroupSha }) { if (!FULL_SHA.test(sourceHeadSha)) { throw new Error("Merge queue source commit is malformed"); } + if (!FULL_SHA.test(baseHeadSha)) { + throw new Error("Merge queue base commit is malformed"); + } } function trustedReviewGateReviewer(status, pullNumber, baseBinding) { @@ -852,164 +1111,381 @@ function latestReviewGateStatusForPull(statuses, pullNumber) { ); } -/** Reuse a successful exact-head review for a synthetic merge queue commit. */ -export async function publishMergeGroupReviewStatus({ +const ACTIVE_MERGE_QUEUE_BINDING_QUERY = ` + query ActiveMergeQueueBinding($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + number + state + headRefOid + baseRefName + mergeQueueEntry { + baseCommit { oid } + headCommit { oid } + } + } + } + } +`; + +async function requireActiveMergeQueueBinding({ github, owner, repo, pullNumber, sourceHeadSha, + baseHeadSha, mergeGroupSha, }) { - let failure; - let pullUrl = `https://github.com/${owner}/${repo}/pull/${pullNumber}`; + const response = await github.graphql(ACTIVE_MERGE_QUEUE_BINDING_QUERY, { + owner, + repo, + number: pullNumber, + }); + const pull = response?.repository?.pullRequest; + const entry = pull?.mergeQueueEntry; + const liveBaseHeadSha = entry?.baseCommit?.oid; + const queueHeadSha = entry?.headCommit?.oid; + const baseRef = pull?.baseRefName; + const normalizedBaseHeadSha = baseHeadSha.toLowerCase(); + if ( + pull?.number !== pullNumber || pull?.state !== "OPEN" || + pull?.headRefOid?.toLowerCase() !== sourceHeadSha.toLowerCase() || + liveBaseHeadSha?.toLowerCase() !== normalizedBaseHeadSha || + queueHeadSha?.toLowerCase() !== mergeGroupSha.toLowerCase() || + typeof baseRef !== "string" || baseRef.length === 0 || + baseRef.length > 1024 || baseRef.includes("\0") + ) { + throw new Error( + "Merge group is not bound to the current pull request head", + ); + } + const ref = + `heads/gh-readonly-queue/${baseRef}/pr-${pullNumber}-${normalizedBaseHeadSha}`; + const parsed = parseMergeQueuePullNumber(`refs/${ref}`); + if ( + parsed?.pullNumber !== pullNumber || + parsed?.baseHeadSha !== normalizedBaseHeadSha + ) { + throw new Error("Merge queue ref identity is malformed"); + } + const liveRef = await github.rest.git.getRef({ owner, repo, ref }); + if ( + liveRef?.data?.object?.sha?.toLowerCase() !== mergeGroupSha.toLowerCase() + ) { + throw new Error("Merge queue ref no longer targets this merge group"); + } +} + +async function readMergeGroupSourceEvidence({ + github, + common, + pullNumber, + sourceHeadSha, +}) { + const [reviews, comments, events, statuses, timeline] = await Promise.all([ + collectAll( + github, + github.rest.pulls.listReviews, + { ...common, pull_number: pullNumber }, + "source reviews", + ), + collectAll( + github, + github.rest.issues.listComments, + { ...common, issue_number: pullNumber }, + "source comments", + ), + collectAll( + github, + github.rest.issues.listEvents, + { ...common, issue_number: pullNumber }, + "source pull request events", + ), + collectAll( + github, + github.rest.repos.listCommitStatusesForRef, + { ...common, ref: sourceHeadSha }, + "source review statuses", + ), + collectAll( + github, + github.rest.issues.listEventsForTimeline, + { ...common, issue_number: pullNumber }, + "source review timeline", + ), + ]); + return { reviews, comments, events, statuses, timeline }; +} + +async function requireCurrentAutomatedReview({ + github, + common, + evidence, + sourceHeadSha, + pull, + pullNumber, + baseBinding, +}) { + const liveReview = await findAutomatedReview( + evidence, + sourceHeadSha, + (ref) => resolveCommitRef(github, common, ref), + (login) => + isCurrentlyTrustedHuman(github, { + ...common, + login, + pullAuthor: pull?.data?.user?.login, + }), + latestReviewResetTime(evidence.statuses, pullNumber, baseBinding), + ); + if (!liveReview) { + throw new Error("Pull request does not have current review evidence"); + } +} + +function requireTrustedReviewGate(statuses, pullNumber, baseBinding) { + const status = latestReviewGateStatusForPull(statuses, pullNumber); + if (!trustedReviewGateReviewer(status, pullNumber, baseBinding)) { + throw new Error( + "Pull request head does not have a current trusted review gate", + ); + } +} + +async function requireUnchangedSourcePull({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + baseBinding, +}) { + const current = await github.rest.pulls.get({ + owner, + repo, + pull_number: pullNumber, + }); + if ( + current?.data?.state !== "open" || + current?.data?.head?.sha?.toLowerCase() !== sourceHeadSha.toLowerCase() + ) { + throw new Error( + "Pull request head changed while propagating review evidence", + ); + } + if (pullRequestBaseBinding(current?.data) !== baseBinding) { + throw new Error( + "Pull request base changed while propagating review evidence", + ); + } + return current; +} + +async function latestTrustedReviewGateStatus({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + baseBinding, + current, +}) { + const latestStatuses = await collectAll( + github, + github.rest.repos.listCommitStatusesForRef, + { owner, repo, ref: sourceHeadSha }, + "latest source review statuses", + ); + const status = latestReviewGateStatusForPull(latestStatuses, pullNumber); + const reviewer = trustedReviewGateReviewer(status, pullNumber, baseBinding); + if (!reviewer) { + throw new Error( + "Pull request review gate changed while propagating review evidence", + ); + } + if ( + reviewer !== CODEX_LOGIN && + !await isCurrentlyTrustedHuman(github, { + owner, + repo, + login: reviewer, + pullAuthor: current?.data?.user?.login, + }) + ) { + throw new Error( + "Human reviewer is no longer trusted for merge queue reuse", + ); + } + return status; +} + +async function publishSuccessfulMergeGroupReviewStatus({ + github, + owner, + repo, + pullNumber, + mergeGroupSha, + currentReviewStatus, + pullUrl, +}) { + const description = `Reused exact-head review for PR #${pullNumber}`; + await github.rest.repos.createCommitStatus({ + owner, + repo, + sha: mergeGroupSha, + state: "success", + context: AUTOMATED_REVIEW_STATUS_CONTEXT, + description, + target_url: typeof currentReviewStatus.target_url === "string" + ? currentReviewStatus.target_url + : pullUrl, + }); + return { + state: "success", + description, + failure: undefined, + published: true, + }; +} + +async function publishVerifiedMergeGroupReviewStatus({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + baseHeadSha, + mergeGroupSha, + pullUrlRef, +}) { + const pull = await github.rest.pulls.get({ + owner, + repo, + pull_number: pullNumber, + }); + if ( + pull?.data?.state !== "open" || + pull?.data?.head?.sha?.toLowerCase() !== sourceHeadSha.toLowerCase() + ) { + throw new Error("Merge queue pull request changed from its queued head"); + } + const baseBinding = pullRequestBaseBinding(pull?.data); + pullUrlRef.value = pull.data.html_url ?? pullUrlRef.value; + const common = { owner, repo }; + const evidence = await readMergeGroupSourceEvidence({ + github, + common, + pullNumber, + sourceHeadSha, + }); + await requireCurrentAutomatedReview({ + github, + common, + evidence, + sourceHeadSha, + pull, + pullNumber, + baseBinding, + }); + requireTrustedReviewGate(evidence.statuses, pullNumber, baseBinding); + const current = await requireUnchangedSourcePull({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + baseBinding, + }); + const currentReviewStatus = await latestTrustedReviewGateStatus({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + baseBinding, + current, + }); try { - assertMergeGroupInputs({ pullNumber, sourceHeadSha, mergeGroupSha }); - const pull = await github.rest.pulls.get({ + await requireActiveMergeQueueBinding({ + github, owner, repo, - pull_number: pullNumber, - }); - if ( - pull?.data?.state !== "open" || - pull?.data?.head?.sha?.toLowerCase() !== sourceHeadSha.toLowerCase() - ) { - throw new Error("Merge queue pull request changed from its queued head"); - } - const baseBinding = pullRequestBaseBinding(pull?.data); - pullUrl = pull.data.html_url ?? pullUrl; - const common = { owner, repo }; - const [reviews, comments, events, statuses, timeline] = await Promise.all([ - collectAll( - github, - github.rest.pulls.listReviews, - { ...common, pull_number: pullNumber }, - "source reviews", - ), - collectAll( - github, - github.rest.issues.listComments, - { ...common, issue_number: pullNumber }, - "source comments", - ), - collectAll( - github, - github.rest.issues.listEvents, - { ...common, issue_number: pullNumber }, - "source pull request events", - ), - collectAll( - github, - github.rest.repos.listCommitStatusesForRef, - { ...common, ref: sourceHeadSha }, - "source review statuses", - ), - collectAll( - github, - github.rest.issues.listEventsForTimeline, - { ...common, issue_number: pullNumber }, - "source review timeline", - ), - ]); - const liveReview = await findAutomatedReview( - { reviews, comments, events, timeline }, + pullNumber, sourceHeadSha, - (ref) => resolveCommitRef(github, common, ref), - (login) => - isCurrentlyTrustedHuman(github, { - ...common, - login, - pullAuthor: pull?.data?.user?.login, - }), - latestReviewResetTime(statuses, pullNumber, baseBinding), - ); - if (!liveReview) { - throw new Error("Pull request does not have current review evidence"); - } - let currentReviewStatus = latestReviewGateStatusForPull( - statuses, + baseHeadSha, + mergeGroupSha, + }); + } catch (error) { + return { + state: "failure", + description: undefined, + failure: error instanceof Error ? error : new Error(String(error)), + published: false, + }; + } + return publishSuccessfulMergeGroupReviewStatus({ + github, + owner, + repo, + pullNumber, + mergeGroupSha, + currentReviewStatus, + pullUrl: pullUrlRef.value, + }); +} + +/** Reuse a successful exact-head review for a synthetic merge queue commit. */ +export async function publishMergeGroupReviewStatus({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + baseHeadSha, + mergeGroupSha, +}) { + const pullUrlRef = { + value: `https://github.com/${owner}/${repo}/pull/${pullNumber}`, + }; + try { + assertMergeGroupInputs({ pullNumber, - ); - if ( - !trustedReviewGateReviewer( - currentReviewStatus, - pullNumber, - baseBinding, - ) - ) { - throw new Error( - "Pull request head does not have a current trusted review gate", - ); - } - const current = await github.rest.pulls.get({ + sourceHeadSha, + baseHeadSha, + mergeGroupSha, + }); + await requireActiveMergeQueueBinding({ + github, owner, repo, - pull_number: pullNumber, + pullNumber, + sourceHeadSha, + baseHeadSha, + mergeGroupSha, }); - if ( - current?.data?.state !== "open" || - current?.data?.head?.sha?.toLowerCase() !== sourceHeadSha.toLowerCase() - ) { - throw new Error( - "Pull request head changed while propagating review evidence", - ); - } - if (pullRequestBaseBinding(current?.data) !== baseBinding) { - throw new Error( - "Pull request base changed while propagating review evidence", - ); - } - const latestStatuses = await collectAll( + } catch (error) { + return { + state: "failure", + description: undefined, + failure: error instanceof Error ? error : new Error(String(error)), + published: false, + }; + } + + try { + return await publishVerifiedMergeGroupReviewStatus({ github, - github.rest.repos.listCommitStatusesForRef, - { owner, repo, ref: sourceHeadSha }, - "latest source review statuses", - ); - const latestReviewStatus = latestReviewGateStatusForPull( - latestStatuses, - pullNumber, - ); - const latestReviewer = trustedReviewGateReviewer( - latestReviewStatus, - pullNumber, - baseBinding, - ); - if (!latestReviewer) { - throw new Error( - "Pull request review gate changed while propagating review evidence", - ); - } - if ( - latestReviewer !== CODEX_LOGIN && - !await isCurrentlyTrustedHuman(github, { - owner, - repo, - login: latestReviewer, - pullAuthor: current?.data?.user?.login, - }) - ) { - throw new Error( - "Human reviewer is no longer trusted for merge queue reuse", - ); - } - currentReviewStatus = latestReviewStatus; - const description = `Reused exact-head review for PR #${pullNumber}`; - await github.rest.repos.createCommitStatus({ owner, repo, - sha: mergeGroupSha, - state: "success", - context: AUTOMATED_REVIEW_STATUS_CONTEXT, - description, - target_url: typeof currentReviewStatus.target_url === "string" - ? currentReviewStatus.target_url - : pullUrl, + pullNumber, + sourceHeadSha, + baseHeadSha, + mergeGroupSha, + pullUrlRef, }); - return { state: "success", description, failure: undefined }; } catch (error) { - failure = error instanceof Error ? error : new Error(String(error)); - } - if (FULL_SHA.test(mergeGroupSha)) { + const failure = error instanceof Error ? error : new Error(String(error)); await github.rest.repos.createCommitStatus({ owner, repo, @@ -1017,10 +1493,52 @@ export async function publishMergeGroupReviewStatus({ state: "failure", context: AUTOMATED_REVIEW_STATUS_CONTEXT, description: "Could not reuse an exact-head review", - target_url: pullUrl, + target_url: pullUrlRef.value, }); + return { + state: "failure", + description: undefined, + failure, + published: true, + }; + } +} + +async function reconcileMergeQueueTarget({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + target, +}) { + const result = await publishMergeGroupReviewStatus({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + baseHeadSha: target.baseHeadSha, + mergeGroupSha: target.mergeGroupSha, + }); + if (result.state !== "failure" || result.published === true) { + return { result }; } - return { state: "failure", description: undefined, failure }; + // No replacement status reached this synthetic commit, so any prior + // success it carries would keep satisfying the queue's required check. + // Skip only when the exact queue ref demonstrably no longer targets the + // commit (a confirmed identity change); an operational lookup failure must + // fail reconciliation so its independent invalidation path takes over. + const liveTarget = await resolveQueueRefTarget( + github, + { owner, repo }, + target.ref, + ); + if (liveTarget?.toLowerCase() !== target.key) return {}; + return { + unpublished: result.failure ?? + new Error("Merge queue review status was not replaced"), + }; } /** Reconcile copied review proof on every active queue ref for one source. */ @@ -1050,34 +1568,34 @@ export async function reconcileActiveMergeGroupReviewStatuses({ { owner, repo, - ref: - `heads/gh-readonly-queue/${baseRef}/pr-${pullNumber}-${sourceHeadSha}`, + ref: `heads/gh-readonly-queue/${baseRef}/pr-${pullNumber}-`, }, "merge queue refs", ); const results = []; const seen = new Set(); + const unpublished = []; for (const queueRef of refs) { - const parsed = parseMergeQueuePullNumber(queueRef?.ref); - const mergeGroupSha = queueRef?.object?.sha; - if ( - parsed?.pullNumber !== pullNumber || - parsed?.sourceHeadSha !== sourceHeadSha.toLowerCase() - ) continue; - if (!FULL_SHA.test(mergeGroupSha ?? "")) { - throw new Error("Merge queue ref has a malformed commit"); - } - if (seen.has(mergeGroupSha.toLowerCase())) continue; - seen.add(mergeGroupSha.toLowerCase()); - results.push( - await publishMergeGroupReviewStatus({ - github, - owner, - repo, - pullNumber, - sourceHeadSha, - mergeGroupSha, - }), + const target = mergeQueueTarget(queueRef, pullNumber); + if (!target || seen.has(target.key)) continue; + seen.add(target.key); + const outcome = await reconcileMergeQueueTarget({ + github, + owner, + repo, + pullNumber, + sourceHeadSha, + target, + }); + if (outcome.unpublished) unpublished.push(outcome.unpublished); + if (outcome.result) results.push(outcome.result); + } + if (unpublished.length > 0) { + throw new Error( + `Review proof was not replaced on ${unpublished.length} active merge ` + + `queue commit(s): ${ + unpublished.map((error) => error.message).join("; ") + }`, ); } return results; diff --git a/scripts/ci/automated-review-gate.test.ts b/scripts/ci/automated-review-gate.test.ts index eafa4ae67a..251b3b2160 100644 --- a/scripts/ci/automated-review-gate.test.ts +++ b/scripts/ci/automated-review-gate.test.ts @@ -17,10 +17,14 @@ import { reconcileActiveMergeGroupReviewStatuses, requestAutomatedReview, reviewBaseBinding, + selectMergeGroupFailureStatusBoundary, + shouldPreserveLaterMergeGroupSuccess, } from "./automated-review-gate.mjs"; const HEAD = "a4804e5b9a0c9c45da7c4866d9eb317c878b029c"; const OTHER_HEAD = "d258d506fede01c84b61bc40488059447d755a5a"; +const BASE_HEAD = "e724246c0e05c8dcf0db41f024f4592128222937"; +const NEW_HEAD = "b8459394dd5bac3a6736ee4c7723d7f291abb382"; const BASE_REPOSITORY_ID = 1_101_259_327; const BASE_REF = "main"; const OTHER_BASE_REF = "release"; @@ -91,6 +95,9 @@ function codexComment( ref = HEAD.slice(0, 10), overrides: Record = {}, ) { + const createdAt = typeof overrides.created_at === "string" + ? overrides.created_at + : "2026-08-25T08:00:00Z"; return { user: bot("chatgpt-codex-connector[bot]", CODEX_ID), body: [ @@ -98,6 +105,8 @@ function codexComment( `**Reviewed commit:** \`${ref}\``, ].join("\n\n"), html_url: "https://example.test/comment", + created_at: createdAt, + updated_at: createdAt, ...overrides, }; } @@ -128,6 +137,25 @@ function associatedPull(overrides: Record = {}) { }; } +function activeQueueBinding(overrides: Record = {}) { + return { + repository: { + pullRequest: { + number: 1, + state: "OPEN", + headRefOid: HEAD, + baseRefName: BASE_REF, + mergeQueueEntry: { + state: "AWAITING_CHECKS", + baseCommit: { oid: BASE_HEAD }, + headCommit: { oid: OTHER_HEAD }, + }, + ...overrides, + }, + }, + }; +} + function record(value: unknown, label: string): Record { if (typeof value !== "object" || value === null || Array.isArray(value)) { throw new TypeError(`${label} must be a record`); @@ -211,6 +239,49 @@ describe("automated review evidence", () => { ); }); + it("lets a later exact-head no-findings comment supersede a Codex finding", async () => { + assertEquals( + (await findAutomatedReview( + { + reviews: [review({ + state: "COMMENTED", + body: "P1: An earlier exact-head review reported a false positive.", + submitted_at: "2026-08-25T08:00:00Z", + })], + comments: [codexComment(HEAD.slice(0, 10), { + created_at: "2026-08-25T08:00:01Z", + })], + }, + HEAD, + () => Promise.resolve(HEAD), + ))?.source, + "codex-comment", + ); + assertEquals( + (await findAutomatedReview( + { + reviews: [review({ + id: 101, + state: "COMMENTED", + body: "P1: An earlier exact-head review reported a false positive.", + submitted_at: "2026-08-25T08:00:00Z", + })], + comments: [codexComment(HEAD.slice(0, 10), { + id: 102, + created_at: "2026-08-25T08:00:00Z", + })], + timeline: [ + { event: "reviewed", id: 101 }, + { event: "commented", id: 102 }, + ], + }, + HEAD, + () => Promise.resolve(HEAD), + ))?.source, + "codex-comment", + ); + }); + it("lets an exact-head Codex finding comment supersede an earlier no-findings comment", async () => { assertEquals( await findAutomatedReview( @@ -225,6 +296,201 @@ describe("automated review evidence", () => { ); }); + it("treats an edited Codex finding comment as newer than a later-created clean verdict", async () => { + assertEquals( + await findAutomatedReview( + { + reviews: [], + comments: [ + codexFindingComment(HEAD.slice(0, 10), { + id: 100, + created_at: "2026-08-25T08:00:00Z", + updated_at: "2026-08-25T08:00:02Z", + }), + codexComment(HEAD.slice(0, 10), { + id: 101, + created_at: "2026-08-25T08:00:01Z", + updated_at: "2026-08-25T08:00:01Z", + }), + ], + }, + HEAD, + () => Promise.resolve(HEAD), + ), + undefined, + ); + }); + + it("treats a same-second verdict after an edited finding as ambiguous", async () => { + const findingId = 100; + const verdictId = 101; + const finding = codexFindingComment(HEAD.slice(0, 10), { + id: findingId, + created_at: "2026-08-25T08:00:00Z", + updated_at: "2026-08-25T08:00:02Z", + }); + const verdict = codexComment(HEAD.slice(0, 10), { + id: verdictId, + created_at: "2026-08-25T08:00:02Z", + updated_at: "2026-08-25T08:00:02Z", + }); + assertEquals( + await findAutomatedReview( + { + reviews: [], + comments: [finding, verdict], + timeline: [ + { event: "commented", id: findingId }, + { event: "commented", id: verdictId }, + ], + }, + HEAD, + () => Promise.resolve(HEAD), + ), + undefined, + ); + }); + + it("does not let an edited clean verdict grant success", async () => { + assertEquals( + await findAutomatedReview( + { + reviews: [], + comments: [ + codexComment(HEAD.slice(0, 10), { + id: 100, + created_at: "2026-08-25T08:00:00Z", + updated_at: "2026-08-25T08:00:03Z", + }), + ], + }, + HEAD, + () => Promise.resolve(HEAD), + ), + undefined, + ); + }); + + it("requires clean verdicts to have a valid creation timestamp", async () => { + for (const createdAt of [undefined, "", "not-a-date"]) { + assertEquals( + await findAutomatedReview( + { + reviews: [], + comments: [ + codexComment(HEAD.slice(0, 10), { + created_at: createdAt, + updated_at: "2026-08-25T08:00:00Z", + }), + ], + }, + HEAD, + () => Promise.resolve(HEAD), + ), + undefined, + `created_at ${createdAt}`, + ); + } + }); + + it("requires clean verdicts to have a matching valid update timestamp", async () => { + for (const updatedAt of [undefined, "", "not-a-date"]) { + assertEquals( + await findAutomatedReview( + { + reviews: [], + comments: [ + codexComment(HEAD.slice(0, 10), { + created_at: "2026-08-25T08:00:00Z", + updated_at: updatedAt, + }), + ], + }, + HEAD, + () => Promise.resolve(HEAD), + ), + undefined, + `updated_at ${updatedAt}`, + ); + } + }); + + it("does not let an edited clean verdict supersede a finding after a base boundary", async () => { + assertEquals( + await findAutomatedReview( + { + reviews: [], + comments: [ + codexComment(HEAD.slice(0, 10), { + id: 100, + created_at: "2026-08-25T08:00:00Z", + updated_at: "2026-08-25T08:00:03Z", + }), + codexFindingComment(HEAD.slice(0, 10), { + id: 101, + created_at: "2026-08-25T08:00:02Z", + updated_at: "2026-08-25T08:00:02Z", + }), + ], + events: [{ + event: "base_ref_changed", + created_at: "2026-08-25T08:00:01Z", + }], + }, + HEAD, + () => Promise.resolve(HEAD), + ), + undefined, + ); + }); + + it("lets only a provably later unedited clean verdict supersede an earlier finding", async () => { + assertEquals( + (await findAutomatedReview( + { + reviews: [], + comments: [ + codexFindingComment(HEAD.slice(0, 10), { + id: 100, + created_at: "2026-08-25T08:00:00Z", + updated_at: "2026-08-25T08:00:00Z", + }), + codexComment(HEAD.slice(0, 10), { + id: 101, + created_at: "2026-08-25T08:00:01Z", + updated_at: "2026-08-25T08:00:01Z", + }), + ], + }, + HEAD, + () => Promise.resolve(HEAD), + ))?.source, + "codex-comment", + ); + assertEquals( + await findAutomatedReview( + { + reviews: [], + comments: [ + codexFindingComment(HEAD.slice(0, 10), { + id: 100, + created_at: "2026-08-25T08:00:00Z", + updated_at: "2026-08-25T08:00:00Z", + }), + codexComment(HEAD.slice(0, 10), { + id: 101, + created_at: "2026-08-25T08:00:00Z", + updated_at: "2026-08-25T08:00:00Z", + }), + ], + }, + HEAD, + () => Promise.resolve(HEAD), + ), + undefined, + ); + }); + it("lets an exact-head Codex finding comment override a Codex approval", async () => { assertEquals( await findAutomatedReview( @@ -750,6 +1016,7 @@ function githubFixture(options: { headResponses?: string[]; pullResponses?: Record[]; commit?: string | undefined; + commitResponses?: (string | undefined)[]; commitError?: Error; failAfterFirstPage?: string; pullError?: Error; @@ -757,6 +1024,11 @@ function githubFixture(options: { permissionError?: Error; pullAuthor?: string; draft?: boolean; + queueBindings?: Record[]; + queueBindingError?: Error; + queueRefHeads?: (string | undefined)[]; + queueRefError?: Error; + statusIds?: unknown[]; } = {}) { const endpoints = { reviews: () => undefined, @@ -767,9 +1039,26 @@ function githubFixture(options: { timeline: () => undefined, }; const published: Record[] = []; + const refReads: Record[] = []; + const graphqlReads: { query: unknown; variables: unknown }[] = []; const pageReads = new Map(); let pullRead = 0; + let commitRead = 0; + let queueBindingRead = 0; + let queueRefRead = 0; + let statusWrite = 0; const github = { + graphql: (query: unknown, variables: unknown) => { + graphqlReads.push({ query, variables }); + if (options.queueBindingError) { + return Promise.reject(options.queueBindingError); + } + const responses = options.queueBindings ?? [activeQueueBinding()]; + const data = responses[ + Math.min(queueBindingRead++, responses.length - 1) + ]; + return Promise.resolve(data); + }, paginate: { async *iterator(endpoint: unknown) { const name = Object.entries(endpoints).find(([, value]) => @@ -815,13 +1104,26 @@ function githubFixture(options: { listEvents: endpoints.events, listEventsForTimeline: endpoints.timeline, }, - git: { listMatchingRefs: endpoints.refs }, + git: { + listMatchingRefs: endpoints.refs, + getRef: (parameters: Record) => { + refReads.push(parameters); + if (options.queueRefError) { + return Promise.reject(options.queueRefError); + } + const heads = options.queueRefHeads ?? [OTHER_HEAD]; + const sha = heads[Math.min(queueRefRead++, heads.length - 1)]; + return Promise.resolve({ data: { object: { sha } } }); + }, + }, repos: { listCommitStatusesForRef: endpoints.statuses, - getCommit: () => - options.commitError - ? Promise.reject(options.commitError) - : Promise.resolve({ data: { sha: options.commit } }), + getCommit: () => { + if (options.commitError) return Promise.reject(options.commitError); + const commits = options.commitResponses ?? [options.commit]; + const sha = commits[Math.min(commitRead++, commits.length - 1)]; + return Promise.resolve({ data: { sha } }); + }, getCollaboratorPermissionLevel: () => options.permissionError ? Promise.reject(options.permissionError) @@ -833,12 +1135,17 @@ function githubFixture(options: { }), createCommitStatus: (value: Record) => { published.push(value); - return Promise.resolve({ data: value }); + const statusIds = options.statusIds ?? + [1000 + published.length]; + const id = statusIds[Math.min(statusWrite++, statusIds.length - 1)]; + return Promise.resolve({ + data: id === undefined ? { ...value } : { ...value, id }, + }); }, }, }, }; - return { github, published }; + return { github, published, refReads, graphqlReads }; } describe("automated review publication", () => { @@ -861,6 +1168,7 @@ describe("automated review publication", () => { pullUrl: "https://example.test/pr/1", }); assertEquals(result.state, "success"); + assertEquals(result.statusId, 1001); assertEquals(fixture.published[0]?.sha, HEAD); assertEquals( fixture.published[0]?.description, @@ -870,6 +1178,262 @@ describe("automated review publication", () => { ); }); + it("fails a source success whose status id is missing", async () => { + const fixture = githubFixture({ + pages: { + reviews: [[]], + comments: [[codexComment()]], + events: [[]], + }, + headResponses: [HEAD], + commit: HEAD, + statusIds: [undefined, 1002], + }); + const result = await publishAutomatedReviewStatus({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + headSha: HEAD, + pullUrl: "https://example.test/pr/1", + }); + + assertEquals(result.state, "failure"); + assertEquals(result.statusId, 1002); + assertEquals(fixture.published[0]?.state, "success"); + assertEquals(fixture.published[1]?.state, "failure"); + }); + + it("fails a source success whose status id is invalid or non-positive", async () => { + for (const statusId of [0, -1, 1.5, "1001"]) { + const fixture = githubFixture({ + pages: { + reviews: [[]], + comments: [[codexComment()]], + events: [[]], + }, + headResponses: [HEAD], + commit: HEAD, + statusIds: [statusId, 1002], + }); + const result = await publishAutomatedReviewStatus({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + headSha: HEAD, + pullUrl: "https://example.test/pr/1", + }); + + assertEquals(result.state, "failure", `status id ${statusId}`); + assertEquals(result.statusId, 1002, `status id ${statusId}`); + assertEquals(fixture.published[0]?.state, "success"); + assertEquals(fixture.published[1]?.state, "failure"); + } + }); + + it("attempts a failure status before rejecting a malformed fallback id", async () => { + const fixture = githubFixture({ + pages: { + reviews: [[]], + comments: [[codexComment()]], + events: [[]], + }, + headResponses: [HEAD], + commit: HEAD, + statusIds: [undefined, undefined], + }); + + await assertRejects( + () => + publishAutomatedReviewStatus({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + headSha: HEAD, + pullUrl: "https://example.test/pr/1", + }), + Error, + "Published review status identity is malformed", + ); + assertEquals(fixture.published[0]?.state, "success"); + assertEquals(fixture.published[1]?.state, "failure"); + }); + + it("does not let an old-head failure close a newer queued commit", async () => { + const fixture = githubFixture({ + commit: OTHER_HEAD, + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/main/pr-1-${BASE_HEAD}`, + object: { sha: BASE_HEAD }, + }]], + }, + }); + const result = await publishReviewResolutionFailure({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + }); + assertEquals(result, { queueFailures: 0, skipped: true }); + assertEquals(fixture.published, []); + }); + + it("does not cross queue ownership when the head changes after failure begins", async () => { + const fixture = githubFixture({ + commitResponses: [HEAD, HEAD, NEW_HEAD], + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/main/pr-1-${BASE_HEAD}`, + object: { sha: OTHER_HEAD }, + }]], + }, + queueBindings: [activeQueueBinding({ headRefOid: NEW_HEAD })], + }); + const result = await publishReviewResolutionFailure({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + }); + assertEquals(result, { queueFailures: 0, skipped: false }); + assertEquals( + fixture.published.map((status) => [status.sha, status.state]), + [[HEAD, "failure"]], + ); + }); + + it("propagates operational queue ownership lookup failures", async () => { + const fixture = githubFixture({ + commit: HEAD, + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/main/pr-1-${BASE_HEAD}`, + object: { sha: OTHER_HEAD }, + }]], + }, + queueBindingError: Object.assign( + new Error("queue binding unavailable"), + { status: 503 }, + ), + queueRefError: Object.assign(new Error("queue ref unavailable"), { + status: 503, + }), + }); + + await assertRejects( + () => + publishReviewResolutionFailure({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + }), + Error, + "queue ref unavailable", + ); + assertEquals( + fixture.published.map((status) => [status.sha, status.state]), + [[HEAD, "failure"]], + "an ownership outage must fail the job instead of reporting queue invalidation complete", + ); + }); + + it("skips only a confirmed missing queue ref", async () => { + const fixture = githubFixture({ + commit: HEAD, + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/main/pr-1-${BASE_HEAD}`, + object: { sha: OTHER_HEAD }, + }]], + }, + queueBindingError: Object.assign( + new Error("queue binding unavailable"), + { status: 503 }, + ), + queueRefError: Object.assign(new Error("queue ref not found"), { + status: 404, + }), + }); + + assertEquals( + await publishReviewResolutionFailure({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + }), + { queueFailures: 0, skipped: false }, + ); + }); + + it("rejects a malformed successful queue ref lookup", async () => { + const fixture = githubFixture({ + commit: HEAD, + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/main/pr-1-${BASE_HEAD}`, + object: { sha: OTHER_HEAD }, + }]], + }, + queueBindingError: Object.assign( + new Error("queue binding unavailable"), + { status: 503 }, + ), + queueRefHeads: [undefined], + }); + + await assertRejects( + () => + publishReviewResolutionFailure({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + }), + Error, + "Merge queue ref response has a malformed commit", + ); + }); + + it("rejects a malformed final source ownership lookup", async () => { + const fixture = githubFixture({ + commitResponses: [HEAD, HEAD, undefined], + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/main/pr-1-${BASE_HEAD}`, + object: { sha: OTHER_HEAD }, + }]], + }, + }); + + await assertRejects( + () => + publishReviewResolutionFailure({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + }), + Error, + "Commit ref response has a malformed commit", + ); + assertEquals( + fixture.published.map((status) => [status.sha, status.state]), + [[HEAD, "failure"]], + "a malformed final source response must not leave queue invalidation looking complete", + ); + }); + it("accepts an exact-head trusted human approval as the escalation path", async () => { const fixture = githubFixture({ pages: { @@ -1360,42 +1924,134 @@ describe("automated review publication", () => { describe("merge queue review propagation", () => { it("extracts only a fully qualified merge-queue pull request ref", () => { assertEquals( - parseMergeQueuePullNumber( - `refs/heads/gh-readonly-queue/main/pr-4135-${OTHER_HEAD}`, - ), - { pullNumber: 4135, sourceHeadSha: OTHER_HEAD }, + parseMergeQueuePullNumber( + `refs/heads/gh-readonly-queue/main/pr-4135-${OTHER_HEAD}`, + ), + { pullNumber: 4135, baseHeadSha: OTHER_HEAD }, + ); + for ( + const ref of [ + `refs/heads/main/pr-4135-${OTHER_HEAD}`, + "refs/heads/gh-readonly-queue/main/pr-0-deadbeef", + `refs/heads/gh-readonly-queue/main/pr-4135-${OTHER_HEAD}extra`, + `refs/heads/gh-readonly-queue/main/not-pr-4135-${OTHER_HEAD}`, + "", + ] + ) assertEquals(parseMergeQueuePullNumber(ref), undefined); + }); + + it("preserves only a pinned success published after the captured boundary", () => { + const later = { + id: 101, + context: "Automated review", + state: "success", + creator: bot("github-actions[bot]", GITHUB_ACTIONS_ID), + description: "Reused exact-head review for PR #1", + }; + const shouldPreserve = (status: Record, boundary = 100) => + shouldPreserveLaterMergeGroupSuccess({ + latestStatus: status, + reconciliationStatusId: boundary, + pullNumber: 1, + }); + + assertEquals(shouldPreserve(later), true); + assertEquals(shouldPreserve({ ...later, id: 100 }), false); + assertEquals( + shouldPreserve({ ...later, id: 99 }), + true, + "the API's reverse-chronological order, not numeric ID order, proves recency", + ); + assertEquals(shouldPreserve({ ...later, state: "failure" }), false); + assertEquals( + shouldPreserve({ + ...later, + creator: bot("github-actions[bot]", GITHUB_ACTIONS_ID + 1), + }), + false, + ); + assertEquals( + shouldPreserve({ + ...later, + description: "Reused exact-head review for PR #2", + }), + false, + ); + }); + + it("uses the boundary captured by the publisher that failed", () => { + const boundary = (targetResult: string, publisherStatusId?: number) => + selectMergeGroupFailureStatusBoundary({ + targetResult, + targetStatusId: 100, + publisherStatusId, + }); + const success = { + id: 101, + context: "Automated review", + state: "success", + creator: bot("github-actions[bot]", GITHUB_ACTIONS_ID), + description: "Reused exact-head review for PR #1", + }; + + assertEquals(boundary("failure", undefined), 100); + assertEquals(boundary("success", 101), 101); + assertEquals( + boundary("success", undefined), + 100, + "a publisher without an output falls back to the resolver boundary", + ); + assertEquals( + shouldPreserveLaterMergeGroupSuccess({ + latestStatus: success, + reconciliationStatusId: boundary("success", 101), + pullNumber: 1, + }), + false, + "a success published before the locked publisher starts is not later", + ); + assertEquals( + shouldPreserveLaterMergeGroupSuccess({ + latestStatus: success, + reconciliationStatusId: boundary("success", undefined), + pullNumber: 1, + }), + true, + "a newer success survives when the older publisher produced no boundary", + ); + assertEquals( + shouldPreserveLaterMergeGroupSuccess({ + latestStatus: { ...success, id: 102 }, + reconciliationStatusId: boundary("success", 101), + pullNumber: 1, + }), + true, + "a success published after the locked publisher starts is preserved", ); - for ( - const ref of [ - `refs/heads/main/pr-4135-${OTHER_HEAD}`, - "refs/heads/gh-readonly-queue/main/pr-0-deadbeef", - `refs/heads/gh-readonly-queue/main/pr-4135-${OTHER_HEAD}extra`, - `refs/heads/gh-readonly-queue/main/not-pr-4135-${OTHER_HEAD}`, - "", - ] - ) assertEquals(parseMergeQueuePullNumber(ref), undefined); }); - it("fails the source and active queue refs without a pull lookup", async () => { - const secondQueueHead = "e724246c0e05c8dcf0db41f024f4592128222937"; + it("fails every exact queue ref still owned by the current source", async () => { + const secondQueueHead = BASE_HEAD; const fixture = githubFixture({ + commit: HEAD, pages: { refs: [[ { - ref: `refs/heads/gh-readonly-queue/main/pr-1-${HEAD}`, + ref: `refs/heads/gh-readonly-queue/main/pr-1-${BASE_HEAD}`, object: { sha: OTHER_HEAD }, }, { - ref: `refs/heads/gh-readonly-queue/release/pr-1-${HEAD}`, + ref: `refs/heads/gh-readonly-queue/release/pr-1-${OTHER_HEAD}`, object: { sha: secondQueueHead }, }, { - ref: `refs/heads/gh-readonly-queue/main/pr-2-${HEAD}`, + ref: `refs/heads/gh-readonly-queue/main/pr-2-${BASE_HEAD}`, object: { sha: secondQueueHead }, }, ]], }, pullError: new Error("pull lookup unavailable"), + queueRefHeads: [OTHER_HEAD, secondQueueHead], }); const result = await publishReviewResolutionFailure({ github: fixture.github, @@ -1429,6 +2085,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(result.state, "success"); @@ -1441,6 +2098,127 @@ describe("merge queue review propagation", () => { description: "Reused exact-head review for PR #1", target_url: "https://example.test/review-proof", }]); + assertEquals(fixture.refReads, [ + { + owner: "veryfront", + repo: "veryfront-code", + ref: `heads/gh-readonly-queue/${BASE_REF}/pr-1-${BASE_HEAD}`, + }, + { + owner: "veryfront", + repo: "veryfront-code", + ref: `heads/gh-readonly-queue/${BASE_REF}/pr-1-${BASE_HEAD}`, + }, + ]); + assertEquals( + fixture.graphqlReads.map(({ variables }) => variables), + [ + { owner: "veryfront", repo: "veryfront-code", number: 1 }, + { owner: "veryfront", repo: "veryfront-code", number: 1 }, + ], + ); + assert( + fixture.graphqlReads.every(({ query }) => + typeof query === "string" && + query.includes("query ActiveMergeQueueBinding") && + query.includes("mergeQueueEntry") + ), + "binding checks must use the expected GraphQL operation and fields", + ); + }); + + it("fails closed when the live merge-queue binding does not match", async () => { + const staleBindings = [ + ["source", activeQueueBinding({ headRefOid: OTHER_HEAD })], + [ + "base", + activeQueueBinding({ + mergeQueueEntry: { + state: "AWAITING_CHECKS", + baseCommit: { oid: HEAD }, + headCommit: { oid: OTHER_HEAD }, + }, + }), + ], + [ + "synthetic head", + activeQueueBinding({ + mergeQueueEntry: { + state: "AWAITING_CHECKS", + baseCommit: { oid: BASE_HEAD }, + headCommit: { oid: HEAD }, + }, + }), + ], + ["missing entry", activeQueueBinding({ mergeQueueEntry: null })], + ] as const; + for (const [label, binding] of staleBindings) { + const fixture = githubFixture({ + pages: { + reviews: [[review({ state: "APPROVED" })]], + statuses: [[automatedReviewStatus()]], + }, + queueBindings: [binding], + }); + const result = await publishMergeGroupReviewStatus({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, + mergeGroupSha: OTHER_HEAD, + }); + assertEquals(result.state, "failure", label); + assertEquals( + fixture.published, + [], + `${label}: an unbound synthetic commit must not receive a status`, + ); + } + + const replacedRef = githubFixture({ + pages: { + reviews: [[review({ state: "APPROVED" })]], + statuses: [[automatedReviewStatus()]], + }, + queueRefHeads: [HEAD], + }); + const result = await publishMergeGroupReviewStatus({ + github: replacedRef.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, + mergeGroupSha: OTHER_HEAD, + }); + assertEquals(result.state, "failure"); + assertEquals(replacedRef.published, []); + }); + + it("rechecks the live merge-queue binding before publishing success", async () => { + const fixture = githubFixture({ + pages: { + reviews: [[review({ state: "APPROVED" })]], + statuses: [[automatedReviewStatus()]], + }, + queueBindings: [ + activeQueueBinding(), + activeQueueBinding({ headRefOid: OTHER_HEAD }), + ], + }); + const result = await publishMergeGroupReviewStatus({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, + mergeGroupSha: OTHER_HEAD, + }); + assertEquals(result.state, "failure"); + assertEquals(fixture.published, []); }); it("does not reuse proof that predates a base-ref issue event", async () => { @@ -1469,6 +2247,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); @@ -1504,6 +2283,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(result.state, "success"); @@ -1517,6 +2297,7 @@ describe("merge queue review propagation", () => { reviews: [[review({ state: "APPROVED" })]], statuses: [[automatedReviewStatus()]], }, + queueBindings: [activeQueueBinding({ number: 2 })], pullResponses: [ associatedPull({ number: 2 }), associatedPull({ number: 2 }), @@ -1528,6 +2309,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 2, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(result.state, "failure"); @@ -1555,6 +2337,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(result.state, "success"); @@ -1605,6 +2388,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(result.state, "failure"); @@ -1625,6 +2409,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(result.state, "failure"); @@ -1642,6 +2427,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(sourceDriftResult.state, "failure"); @@ -1652,7 +2438,7 @@ describe("merge queue review propagation", () => { pages: { reviews: [[review({ state: "APPROVED" })]], refs: [[{ - ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${HEAD}`, + ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${BASE_HEAD}`, object: { sha: OTHER_HEAD }, }]], statuses: [[automatedReviewStatus({ state: "pending" })]], @@ -1673,6 +2459,133 @@ describe("merge queue review propagation", () => { assertEquals(fixture.published[0]?.state, "failure"); }); + it("surfaces an unpublished queue failure to the workflow", async () => { + const fixture = githubFixture({ + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${BASE_HEAD}`, + object: { sha: OTHER_HEAD }, + }]], + }, + queueBindingError: Object.assign( + new Error("queue binding unavailable"), + { status: 503 }, + ), + }); + + await assertRejects( + () => + reconcileActiveMergeGroupReviewStatuses({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + baseRef: BASE_REF, + }), + Error, + "queue binding unavailable", + ); + assertEquals( + fixture.published, + [], + "the workflow fallback must handle a failure that could not be published safely", + ); + }); + + it("fails closed when a queue failure cannot reach a live entry", async () => { + const fixture = githubFixture({ + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${BASE_HEAD}`, + object: { sha: OTHER_HEAD }, + }]], + statuses: [[automatedReviewStatus({ state: "pending" })]], + }, + headResponses: [HEAD, HEAD], + queueBindingError: Object.assign( + new Error("queue binding unavailable"), + { status: 503 }, + ), + }); + await assertRejects( + () => + reconcileActiveMergeGroupReviewStatuses({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + baseRef: BASE_REF, + }), + Error, + "Review proof was not replaced on 1 active merge queue commit", + ); + assertEquals(fixture.published, []); + }); + + it("skips an unverifiable entry whose ref no longer targets it", async () => { + const fixture = githubFixture({ + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${BASE_HEAD}`, + object: { sha: OTHER_HEAD }, + }]], + statuses: [[automatedReviewStatus({ state: "pending" })]], + }, + headResponses: [HEAD, HEAD], + queueBindingError: Object.assign( + new Error("queue binding unavailable"), + { status: 503 }, + ), + queueRefHeads: [NEW_HEAD], + }); + const results = await reconcileActiveMergeGroupReviewStatuses({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + baseRef: BASE_REF, + }); + assertEquals(results, []); + assertEquals(fixture.published, []); + }); + + it("propagates an operational ref lookup during the unpublished recheck", async () => { + const fixture = githubFixture({ + pages: { + refs: [[{ + ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${BASE_HEAD}`, + object: { sha: OTHER_HEAD }, + }]], + statuses: [[automatedReviewStatus({ state: "pending" })]], + }, + headResponses: [HEAD, HEAD], + queueBindingError: Object.assign( + new Error("queue binding unavailable"), + { status: 503 }, + ), + queueRefError: Object.assign(new Error("queue ref unavailable"), { + status: 503, + }), + }); + await assertRejects( + () => + reconcileActiveMergeGroupReviewStatuses({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + sourceHeadSha: HEAD, + baseRef: BASE_REF, + }), + Error, + "queue ref unavailable", + ); + assertEquals(fixture.published, []); + }); + it("rechecks source proof immediately before publishing queue success", async () => { const fixture = githubFixture({ pageResponses: { @@ -1690,6 +2603,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(result.state, "failure"); @@ -1721,6 +2635,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(result.state, "failure"); @@ -1760,6 +2675,7 @@ describe("merge queue review propagation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals(result.state, candidate.state); @@ -1946,6 +2862,27 @@ describe("automated review request", () => { }); describe("review proof invalidation", () => { + it("invalidates this run's source success after an unreplaced queue result", async () => { + const fixture = githubFixture({ + commit: HEAD, + headResponses: [HEAD], + pages: { + statuses: [[automatedReviewStatus({ id: 101 })]], + refs: [[]], + }, + }); + const result = await invalidateReviewProof({ + github: fixture.github, + owner: "veryfront", + repo: "veryfront-code", + pullNumber: 1, + }); + + assertEquals(result.skipped, false); + assertEquals(fixture.published[0]?.sha, HEAD); + assertEquals(fixture.published[0]?.state, "failure"); + }); + it("does not overwrite a same-second success published after reconciliation began", async () => { const fixture = githubFixture({ headResponses: [HEAD], @@ -1955,7 +2892,7 @@ describe("review proof invalidation", () => { created_at: "2026-08-25T08:00:00Z", })]], refs: [[{ - ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${HEAD}`, + ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${BASE_HEAD}`, object: { sha: OTHER_HEAD }, }]], }, @@ -1979,6 +2916,7 @@ describe("review proof invalidation", () => { it("invalidates a same-second success that existed when reconciliation began", async () => { const fixture = githubFixture({ + commit: HEAD, headResponses: [HEAD], pages: { statuses: [[automatedReviewStatus({ @@ -2002,10 +2940,11 @@ describe("review proof invalidation", () => { it("closes source and queued gates a dropped reconciliation left behind", async () => { const fixture = githubFixture({ + commit: HEAD, headResponses: [HEAD], pages: { refs: [[{ - ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${HEAD}`, + ref: `refs/heads/gh-readonly-queue/${BASE_REF}/pr-1-${BASE_HEAD}`, object: { sha: OTHER_HEAD }, }]], }, @@ -2087,6 +3026,7 @@ describe("review proof invalidation", () => { repo: "veryfront-code", pullNumber: 1, sourceHeadSha: HEAD, + baseHeadSha: BASE_HEAD, mergeGroupSha: OTHER_HEAD, }); assertEquals( @@ -2360,6 +3300,10 @@ describe("automated review workflow", () => { record(targetJob.outputs, "target outputs").key, "${{ steps.resolve.outputs.key }}", ); + assertEquals( + record(targetJob.outputs, "target outputs").head_sha, + "${{ steps.resolve.outputs.head-sha }}", + ); assertEquals( record(targetJob.outputs, "target outputs").pull_number, "${{ steps.resolve.outputs.pull-number }}", @@ -2368,6 +3312,10 @@ describe("automated review workflow", () => { record(targetJob.outputs, "target outputs").status_id, "${{ steps.resolve.outputs.status-id }}", ); + assertEquals( + record(targetJob.outputs, "target outputs").merge_group_status_id, + "${{ steps.resolve.outputs.merge-group-status-id }}", + ); const targetSteps = targetJob.steps; assert(Array.isArray(targetSteps)); const targetScript = String( @@ -2390,14 +3338,43 @@ describe("automated review workflow", () => { 'context.eventName === "workflow_run"', "Number.isSafeInteger", 'core.setOutput("key"', + 'core.setOutput("head-sha"', 'core.setOutput("pull-number"', 'core.setOutput("status-id"', + '"merge-group-status-id"', "github.rest.git.getRef", "fallbackResponse = await github.rest.pulls.get", "context.payload.pull_request?.head?.sha", "Could not resolve a valid review target commit", + "context.payload.merge_group?.base_sha", + "queueEntry.baseHeadSha", ] ) assert(targetScript.includes(required)); + assert( + targetScript.includes('core.setOutput("key", `pr-${pullNumber}`)'), + "all publishers for one pull request must share a lock across force-pushes", + ); + const earlyMergeGroupLock = + 'core.setOutput("key", `pr-${queueEntry.pullNumber}`)'; + assert( + targetScript.includes(earlyMergeGroupLock) && + targetScript.indexOf(earlyMergeGroupLock) < + targetScript.indexOf("Merge queue base does not match"), + "a parsed merge group must publish its per-pull lock before later resolver failures", + ); + assert( + targetScript.indexOf('"merge-group-status-id"') < + targetScript.indexOf("Merge queue base does not match"), + "the emergency publisher boundary must be captured before later target work", + ); + assert( + !targetScript.includes("if (!headSha)"), + "the resolver must not wrap its unconditional head lookup in a dead guard", + ); + assert( + !targetScript.includes("let headSha = queueEntry?.sourceHeadSha"), + "the queue ref suffix is the base commit, not the source pull request head", + ); assert( targetScript.replaceAll(/\s+/g, " ").includes( 'context.eventName === "workflow_run" && workflowRun?.conclusion !== "success"', @@ -2447,7 +3424,25 @@ describe("automated review workflow", () => { mergeGroupFailureJob.permissions, "merge group target failure permissions", ), - { statuses: "write" }, + { contents: "read", statuses: "write" }, + ); + assertEquals(record(mergeGroupFailureJob.env, "merge group failure env"), { + TARGET_RESULT: "${{ needs.target.result }}", + TARGET_STATUS_ID: + "${{ needs.target.outputs.merge_group_status_id }}", + PUBLISHER_STATUS_ID: "${{ needs.merge_group.outputs.status_id }}", + }); + assertEquals( + record( + mergeGroupFailureJob.concurrency, + "merge group target failure concurrency", + ), + { + group: + "automated-review-${{ needs.target.outputs.key || format('merge-group-{0}', github.event.merge_group.head_ref) }}", + queue: "max", + }, + "the emergency publisher must serialize with the normal per-pull publisher", ); const mergeGroupFailureSteps = mergeGroupFailureJob.steps; assert(Array.isArray(mergeGroupFailureSteps)); @@ -2460,6 +3455,22 @@ describe("automated review workflow", () => { for ( const required of [ "context.payload.merge_group?.head_sha", + "context.payload.merge_group?.head_ref", + "github.rest.repos.listCommitStatusesForRef", + "process.env.TARGET_RESULT", + "process.env.TARGET_STATUS_ID", + "process.env.PUBLISHER_STATUS_ID", + "selectMergeGroupFailureStatusBoundary", + "reconciliationStatusId", + "latestStatus", + "shouldPreserveLaterMergeGroupSuccess", + "latestStatus.id !== reconciliationStatusId", + "automated-review-gate.mjs", + "latestStatus?.creator?.login", + "latestStatus?.creator?.id", + '"github-actions[bot]"', + "41898282", + "`Reused exact-head review for PR #${pullNumber}`", "github.rest.repos.createCommitStatus", 'context: "Automated review"', 'state: "failure"', @@ -2470,6 +3481,12 @@ describe("automated review workflow", () => { "a merge-group resolver failure must close the gate on the synthetic commit", ); } + assert( + mergeGroupFailureScript.indexOf( + "shouldPreserveLaterMergeGroupSuccess", + ) < mergeGroupFailureScript.indexOf("createCommitStatus"), + "an older emergency publisher must preserve a success written after its status boundary", + ); const job = record(jobs.review, "review job"); assertEquals(record(job.permissions, "review permissions"), { @@ -2483,6 +3500,10 @@ describe("automated review workflow", () => { queue: "max", }; assertEquals(job.needs, "target"); + assertEquals(record(job.outputs, "review outputs"), { + force_invalidate: "${{ steps.publish.outputs.force-invalidate }}", + source_status_id: "${{ steps.publish.outputs.source-status-id }}", + }); assertEquals(record(job.concurrency, "review concurrency"), { ...publisherConcurrency, }); @@ -2498,7 +3519,7 @@ describe("automated review workflow", () => { assertTrustedGateLoad(script); assertEquals( record(gate.env, "gate environment").TARGET_SHA, - "${{ needs.target.outputs.key }}", + "${{ needs.target.outputs.head_sha }}", ); assertEquals( record(gate.env, "gate environment").PULL_NUMBER, @@ -2512,6 +3533,37 @@ describe("automated review workflow", () => { assert(script.includes("process.env.PULL_NUMBER")); assert(script.includes("Number.isSafeInteger")); assert(script.includes("result.baseRef")); + assert( + script.includes( + 'entry?.state === "failure" && entry?.published !== true', + ) && script.includes("Review proof was not replaced"), + "an unreplaced merge queue failure must fail the run so the fallback invalidation job fires", + ); + assert( + script.includes('core.setOutput("force-invalidate", "true")'), + "an unreplaced queue failure must tell fallback invalidation to ignore this run's source success", + ); + assert( + script.includes('core.setOutput("source-status-id"') && + script.includes("result.statusId"), + "fallback invalidation must receive the source status written by this failed run", + ); + assert( + script.includes( + 'result.state === "success"', + ) && + script.includes( + "Automated review success did not return a source status identity.", + ), + "a source success without an identity must not be treated as usable review proof", + ); + assert( + script.includes("forceInvalidateCurrentSource") && + script.includes( + "Cannot force invalidation without the source status identity.", + ), + "forced invalidation must require the exact source status boundary", + ); assert(!script.includes("listPullRequestsAssociatedWithCommit")); assert(!script.includes("allowPullRequestReviews")); assert( @@ -2623,19 +3675,35 @@ describe("automated review workflow", () => { assertEquals( record(invalidateJob.env, "invalidate environment"), { - TARGET_SHA: "${{ needs.target.outputs.key }}", + TARGET_SHA: "${{ needs.target.outputs.head_sha }}", PULL_NUMBER: "${{ needs.target.outputs.pull_number }}", - RECONCILIATION_STATUS_ID: - "${{ needs.target.outputs.status_id }}", + RECONCILIATION_STATUS_ID: "${{ needs.target.outputs.status_id }}", + SOURCE_STATUS_ID: "${{ needs.review.outputs.source_status_id }}", + FORCE_INVALIDATE: "${{ needs.review.outputs.force_invalidate }}", }, ); for ( const required of [ "invalidateReviewProof", "reconciliationStatusId", + "process.env.FORCE_INVALIDATE", + "process.env.SOURCE_STATUS_ID", + "forceInvalidate", "publishIndependentFailure", "listMatchingRefs", "createCommitStatus", + "parseMergeQueuePullNumber", + "github.graphql", + "mergeQueueEntry", + "baseCommit", + "headCommit", + "github.rest.git.getRef", + "resolveQueueRefTarget", + "error?.status === 404", + "Merge queue ref response has a malformed commit", + "Pull request head response has a malformed commit", + "finalPull", + 'ref: "heads/gh-readonly-queue/"', "process.env.TARGET_SHA", "process.env.PULL_NUMBER", "Number.isSafeInteger", @@ -2647,10 +3715,21 @@ describe("automated review workflow", () => { "the invalidator must resolve its pull request the way the resolver does", ); } + assert( + !invalidateScript.includes("pr-${pullNumber}-${headSha}"), + "the independent fallback must not treat the queue-ref base suffix as a source head", + ); + assert( + !invalidateScript.includes('forceInvalidate || statusIdText === ""'), + "forced invalidation must not bypass later-success detection", + ); const mergeGroupJob = record(jobs.merge_group, "merge group job"); assertEquals(mergeGroupJob.if, "github.event_name == 'merge_group'"); assertEquals(mergeGroupJob.needs, "target"); + assertEquals(record(mergeGroupJob.outputs, "merge group outputs"), { + status_id: "${{ steps.reuse.outputs.merge-group-status-id }}", + }); assertEquals( record(mergeGroupJob.permissions, "merge group permissions"), { @@ -2672,13 +3751,47 @@ describe("automated review workflow", () => { ).script, ); assertTrustedGateLoad(mergeGroupScript); + assertEquals( + record( + record(mergeGroupSteps[0], "merge group gate").env, + "merge group environment", + ).SOURCE_HEAD_SHA, + "${{ needs.target.outputs.head_sha }}", + ); assert(mergeGroupScript.includes("parseMergeQueuePullNumber")); assert(mergeGroupScript.includes("publishMergeGroupReviewStatus")); assert(mergeGroupScript.includes("process.env.SOURCE_HEAD_SHA")); assert(mergeGroupScript.includes("process.env.PULL_NUMBER")); + assert(mergeGroupScript.includes('"merge-group-status-id"')); + assert(mergeGroupScript.includes("listCommitStatusesForRef")); assert(mergeGroupScript.includes("sourceHeadSha")); assert(mergeGroupScript.includes("context.payload.merge_group.head_ref")); assert(mergeGroupScript.includes("context.payload.merge_group.head_sha")); + assert(mergeGroupScript.includes("context.payload.merge_group.base_sha")); + assert(mergeGroupScript.includes("queueEntry.baseHeadSha")); + assert( + mergeGroupScript.indexOf("listCommitStatusesForRef") < + mergeGroupScript.indexOf("getContent"), + "the publisher must capture its status boundary immediately after acquiring the lock", + ); + assert( + mergeGroupScript.includes("queueEntry.sourceHeadSha"), + "the merge-group job must remain compatible with the trusted pre-fix parser until this PR merges", + ); + for ( + const required of [ + "github.graphql", + "mergeQueueEntry", + "baseCommit", + "headCommit", + "github.rest.git.getRef", + ] + ) { + assert( + mergeGroupScript.includes(required), + "the merge-group job must bind the event to the live queue entry", + ); + } assert( !mergeGroupScript.includes("requestAutomatedReview"), "merge groups must reuse source proof without rerunning Codex",