From 6f7edb36953fa28a3a8ac4da676c19c54007c6c5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 27 Jul 2026 17:11:28 -0400 Subject: [PATCH] fix: make auto-review and PR triage fail safely (#114745) * fix: make auto-review and PR triage fail safely * test: cover fail-closed PR merge review gates * fix: preserve GitHub pending-check review semantics --- scripts/github/pr-ci-sweeper.mjs | 28 ++- scripts/pr-lib/merge.sh | 41 +++- scripts/pr-lib/prepare-core.sh | 7 +- scripts/pr-lib/review-artifacts.mjs | 27 ++- scripts/pr-lib/review.sh | 37 ++- .../bash-tools.exec-host-gateway.test.ts | 9 +- src/agents/bash-tools.exec-host-gateway.ts | 61 ++--- src/agents/bash-tools.exec-host-node.test.ts | 9 +- src/agents/bash-tools.exec-host-node.ts | 53 +++-- src/agents/bash-tools.exec-run.ts | 17 +- src/agents/exec-auto-review.stress.test.ts | 78 +++++-- src/agents/exec-auto-reviewer.test.ts | 53 +++++ src/agents/exec-auto-reviewer.ts | 15 +- test/scripts/pr-ci-sweeper.test.ts | 61 ++++- test/scripts/pr-merge.test.ts | 53 ++++- test/scripts/pr-prepare-gates.test.ts | 44 ++++ .../pr-review-artifact-validation.test.ts | 218 +++++++++++++++++- 17 files changed, 685 insertions(+), 126 deletions(-) diff --git a/scripts/github/pr-ci-sweeper.mjs b/scripts/github/pr-ci-sweeper.mjs index 44f7691ca78e..4339c639565b 100644 --- a/scripts/github/pr-ci-sweeper.mjs +++ b/scripts/github/pr-ci-sweeper.mjs @@ -434,6 +434,12 @@ export async function runPrCiSweeper({ // close history so a racing push or human action wins over the sweep. const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: listed.number }); if (pr.state !== "open" || pr.head.sha !== listed.head.sha) { + results.push({ + number: listed.number, + sha: listed.head.sha.slice(0, 12), + action: "skip", + reason: "changed-during-sweep", + }); core.info(`pr-ci-sweeper: #${listed.number} changed during sweep; leaving it alone`); continue; } @@ -452,17 +458,17 @@ export async function runPrCiSweeper({ sweeperLogins.has(event.actor.login), ).length; const verdict = classifyPrForSweep({ pr, ciRuns, botCloseCount, now }); - results.push({ number: pr.number, sha: pr.head.sha.slice(0, 12), ...verdict }); if (verdict.action !== "refire") { + results.push({ number: pr.number, sha: pr.head.sha.slice(0, 12), ...verdict }); core.info(`pr-ci-sweeper: skip #${pr.number} (${verdict.reason})`); continue; } - refires += 1; if (dryRun) { + refires += 1; + results.push({ number: pr.number, sha: pr.head.sha.slice(0, 12), ...verdict }); core.info(`pr-ci-sweeper: dry-run, would re-fire #${pr.number} (${verdict.reason})`); continue; } - core.info(`pr-ci-sweeper: re-firing CI for #${pr.number} (${verdict.reason})`); // Revalidate immediately before mutating: a human close or a fresh push in // the classify gap must win over the sweep. const { data: fresh } = await github.rest.pulls.get({ owner, repo, pull_number: pr.number }); @@ -472,6 +478,12 @@ export async function runPrCiSweeper({ fresh.auto_merge || fresh.mergeable === false ) { + results.push({ + number: pr.number, + sha: pr.head.sha.slice(0, 12), + action: "skip", + reason: "changed-during-sweep", + }); core.info(`pr-ci-sweeper: #${pr.number} changed during sweep; leaving it alone`); continue; } @@ -484,9 +496,19 @@ export async function runPrCiSweeper({ headSha: fresh.head.sha, }); if (latestRuns.some((run) => run.conclusion !== "startup_failure")) { + results.push({ + number: pr.number, + sha: pr.head.sha.slice(0, 12), + action: "skip", + reason: "ci-attached", + }); core.info(`pr-ci-sweeper: #${pr.number} CI attached during sweep; leaving it alone`); continue; } + // Spend the bounded repair budget only after the exact head still needs a close/reopen. + refires += 1; + results.push({ number: pr.number, sha: pr.head.sha.slice(0, 12), ...verdict }); + core.info(`pr-ci-sweeper: re-firing CI for #${pr.number} (${verdict.reason})`); const knownCloseIds = new Set( events.filter((event) => event.event === "closed").map((event) => event.id), ); diff --git a/scripts/pr-lib/merge.sh b/scripts/pr-lib/merge.sh index 00f043948fee..cc7f8bc12ca6 100644 --- a/scripts/pr-lib/merge.sh +++ b/scripts/pr-lib/merge.sh @@ -137,11 +137,35 @@ merge_verify() { gh pr checks "$pr" --required --watch --fail-fast >.local/merge-checks-watch.log 2>&1 || true local checks_json local checks_err_file + local checks_exit_status checks_err_file=$(mktemp) - checks_json=$(gh pr checks "$pr" --required --json name,bucket,state 2>"$checks_err_file" || true) + if checks_json=$(gh pr checks "$pr" --required --json name,bucket,state 2>"$checks_err_file"); then + checks_exit_status=0 + else + checks_exit_status=$? + fi + # gh documents exit 8 for pending checks even when it emits valid JSON. Let + # the checked evidence below reject pending checks without hiding API errors. + if [ "$checks_exit_status" -ne 0 ] && [ "$checks_exit_status" -ne 8 ]; then + local checks_error + checks_error=$(cat "$checks_err_file") + case "$checks_error" in + "no required checks reported on the '"*"' branch") + # gh reports the valid empty-required set as an error, not a JSON array. + checks_json='[]' + ;; + *) + echo "Merge verify failed: unable to verify the required GitHub checks." >&2 + printf '%s\n' "$checks_error" >&2 + rm -f "$checks_err_file" + return 1 + ;; + esac + fi rm -f "$checks_err_file" - if [ -z "$checks_json" ]; then - checks_json='[]' + if ! printf '%s\n' "$checks_json" | jq -e 'type == "array"' >/dev/null; then + echo "Merge verify failed: GitHub returned invalid required-check evidence." >&2 + return 1 fi local required_count required_count=$(printf '%s\n' "$checks_json" | jq 'length') @@ -197,10 +221,19 @@ merge_run() { enter_worktree "$pr" false local required - for required in .local/review.md .local/review.json .local/prep.md .local/prep.env; do + for required in \ + .local/review.md \ + .local/review.json \ + .local/pr-meta.env \ + .local/pr-meta.json \ + .local/prep.md \ + .local/prep.env + do require_artifact "$required" done + validate_review_artifact_data || return 1 + require_ready_review_recommendation || return 1 merge_verify "$pr" # shellcheck disable=SC1091 source .local/prep.env diff --git a/scripts/pr-lib/prepare-core.sh b/scripts/pr-lib/prepare-core.sh index 7217b6b93702..43c3782069a1 100644 --- a/scripts/pr-lib/prepare-core.sh +++ b/scripts/pr-lib/prepare-core.sh @@ -119,16 +119,15 @@ verify_prep_branch_matches_prepared_head() { prepare_init() { local pr="$1" + # Validate the exact reviewed head before taking the lock past its reversible phase. + review_validate_artifacts "$pr" || return 1 + require_ready_review_recommendation || return 1 mark_pr_operation_side_effects_started enter_worktree "$pr" true require_artifact .local/pr-meta.env require_artifact .local/review.md - if [ ! -s .local/review.json ]; then - echo "WARNING: .local/review.json is missing; structured findings are expected." - fi - local recorded_source_head="" if [ -s .local/prep-context.env ]; then recorded_source_head=$( diff --git a/scripts/pr-lib/review-artifacts.mjs b/scripts/pr-lib/review-artifacts.mjs index f51711f97d84..30fe37a68ac5 100644 --- a/scripts/pr-lib/review-artifacts.mjs +++ b/scripts/pr-lib/review-artifacts.mjs @@ -168,6 +168,17 @@ function validateReviewArtifacts({ review, reviewMarkdown, prMeta }) { const nitFindingsCount = findings.filter( (finding) => isObject(finding) && finding.severity === "NIT", ).length; + if ( + value.recommendation === "READY FOR /prepare-pr" && + findings.some( + (finding) => + isObject(finding) && (finding.severity === "BLOCKER" || finding.severity === "IMPORTANT"), + ) + ) { + add( + "Invalid recommendation in .local/review.json: READY FOR /prepare-pr cannot include BLOCKER or IMPORTANT findings", + ); + } const nitSweep = nitSweepIsObject ? value.nitSweep : {}; const nitSweepPerformedIsBoolean = requireType( @@ -258,7 +269,7 @@ function validateReviewArtifacts({ review, reviewMarkdown, prMeta }) { const runtimeFileCount = prMetaIsValid ? prMeta.files.filter( ({ path }) => - /^(src|extensions|apps)\//u.test(path) && + /^(src|extensions|apps|packages|ui)\//u.test(path) && !/(^|\/)__tests__\/|\.test\.|\.spec\./u.test(path) && !/\.(md|mdx)$/u.test(path), ).length @@ -407,6 +418,20 @@ function validateReviewArtifacts({ review, reviewMarkdown, prMeta }) { if (testsResultIsString) { requireEnum(tests.result, "testsResult", "Invalid tests result in .local/review.json"); } + if (value.recommendation === "READY FOR /prepare-pr" && tests.result === "fail") { + add( + "Invalid recommendation in .local/review.json: READY FOR /prepare-pr cannot include failing tests", + ); + } + if ( + value.recommendation === "READY FOR /prepare-pr" && + runtimeReviewRequired && + tests.result !== "pass" + ) { + add( + "Invalid recommendation in .local/review.json: READY FOR /prepare-pr on runtime changes requires passing tests", + ); + } const docsIsString = requireType( typeof value.docs === "string", diff --git a/scripts/pr-lib/review.sh b/scripts/pr-lib/review.sh index f77cee920081..3ef639e4a822 100644 --- a/scripts/pr-lib/review.sh +++ b/scripts/pr-lib/review.sh @@ -182,16 +182,7 @@ EOF_MD echo "files=.local/review.md .local/review.json" } -review_validate_artifacts() { - local pr="$1" - enter_worktree "$pr" false - require_artifact .local/review.md - require_artifact .local/review.json - require_artifact .local/pr-meta.env - require_artifact .local/pr-meta.json - - review_guard "$pr" - +validate_review_artifact_data() { if ! node "$(review_artifacts_helper_path)" validate \ .local/review.json \ .local/review.md \ @@ -199,6 +190,32 @@ review_validate_artifacts() { then return 1 fi +} + +require_ready_review_recommendation() { + if ! jq -e '.recommendation == "READY FOR /prepare-pr"' .local/review.json >/dev/null; then + echo "PR preparation requires a validated READY FOR /prepare-pr review recommendation." + return 1 + fi +} + +review_validate_artifacts() { + local pr="$1" + # Callers use an OR-list to keep pre-mutation failures reversible; Bash disables + # errexit within that context, so every artifact and exact-head guard must propagate. + enter_worktree "$pr" false || return 1 + require_artifact .local/review.md || return 1 + require_artifact .local/review.json || return 1 + require_artifact .local/pr-meta.env || return 1 + require_artifact .local/pr-meta.json || return 1 + + review_guard "$pr" || return 1 + if [ "${REVIEW_MODE:-}" != "pr" ]; then + echo "Review artifact validation requires the reviewed PR head, not main-baseline mode." + return 1 + fi + + validate_review_artifact_data || return 1 echo "review artifacts validated" print_review_stdout_summary diff --git a/src/agents/bash-tools.exec-host-gateway.test.ts b/src/agents/bash-tools.exec-host-gateway.test.ts index 61475ac26380..4e2f5d9e5b30 100644 --- a/src/agents/bash-tools.exec-host-gateway.test.ts +++ b/src/agents/bash-tools.exec-host-gateway.test.ts @@ -871,13 +871,7 @@ describe("processGatewayAllowlist", () => { it("does not execute after cancellation wins during auto-review", async () => { const command = "echo ok"; await configurePlanBackedCommand({ command }); - let resolveReview: ((decision: Awaited>) => void) | undefined; - const autoReviewer = vi.fn( - () => - new Promise((resolve) => { - resolveReview = resolve; - }), - ); + const autoReviewer = vi.fn(() => new Promise(() => {})); const abortController = new AbortController(); const result = runGatewayAllowlist({ command, @@ -889,7 +883,6 @@ describe("processGatewayAllowlist", () => { await vi.waitFor(() => expect(autoReviewer).toHaveBeenCalledTimes(1)); abortController.abort(new Error("cancelled during review")); - resolveReview?.({ decision: "allow-once", risk: "low", rationale: "allowed" }); await expect(result).rejects.toThrow("cancelled during review"); expect(createAndRegisterDefaultExecApprovalRequestMock).not.toHaveBeenCalled(); diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index 423afdb33e21..65e06defccb5 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -79,6 +79,7 @@ import type { ExecApprovalFollowupOutcome, ExecToolDetails, } from "./bash-tools.exec-types.js"; +import { abortable } from "./embedded-agent-runner/run/abortable.js"; import type { AgentToolResult } from "./runtime/index.js"; /** Full input bundle for gateway-host allowlist and approval processing. */ @@ -797,34 +798,40 @@ export async function processGatewayAllowlist( requiresSecurityAuditSuppressionApproval; if (canAutoReviewApprovalMiss) { const reviewer = params.autoReviewer ?? defaultExecAutoReviewer; - const decision = await reviewer({ - command: params.command, - argv: autoReviewArgv, - resolvedPath: autoReviewResolvedPath, - cwd: params.workdir, - envKeys: Object.keys(params.requestedEnv ?? {}).toSorted(), - host: "gateway", - reason: resolveGatewayAutoReviewReason({ - requiresInlineEvalApproval, - requiresHeredocApproval, - requiresAllowlistPlanApproval, - hostSecurity, - analysisOk, - allowlistSatisfied, - durableApprovalSatisfied, + const pendingDecision = Promise.resolve( + reviewer({ + command: params.command, + argv: autoReviewArgv, + resolvedPath: autoReviewResolvedPath, + cwd: params.workdir, + envKeys: Object.keys(params.requestedEnv ?? {}).toSorted(), + host: "gateway", + reason: resolveGatewayAutoReviewReason({ + requiresInlineEvalApproval, + requiresHeredocApproval, + requiresAllowlistPlanApproval, + hostSecurity, + analysisOk, + allowlistSatisfied, + durableApprovalSatisfied, + }), + analysis: { + parsed: analysisOk, + allowlistMatched: allowlistSatisfied, + durableApprovalMatched: durableApprovalSatisfied, + inlineEval: requiresInlineEvalApproval, + heredoc: requiresHeredocApproval, + }, + agent: { + id: params.agentId, + sessionKey: params.sessionKey, + }, }), - analysis: { - parsed: analysisOk, - allowlistMatched: allowlistSatisfied, - durableApprovalMatched: durableApprovalSatisfied, - inlineEval: requiresInlineEvalApproval, - heredoc: requiresHeredocApproval, - }, - agent: { - id: params.agentId, - sessionKey: params.sessionKey, - }, - }); + ); + // Custom reviewers may never settle; cancellation must not retain approval authority. + const decision = params.signal + ? await abortable(params.signal, pendingDecision) + : await pendingDecision; params.signal?.throwIfAborted(); if ( decision.decision === "allow-once" && diff --git a/src/agents/bash-tools.exec-host-node.test.ts b/src/agents/bash-tools.exec-host-node.test.ts index 443bcfe657a7..dde84dd9115b 100644 --- a/src/agents/bash-tools.exec-host-node.test.ts +++ b/src/agents/bash-tools.exec-host-node.test.ts @@ -1268,13 +1268,7 @@ describe("executeNodeHostCommand", () => { }); it("does not invoke the node after cancellation wins during auto-review", async () => { - let resolveReview: ((decision: Awaited>) => void) | undefined; - const autoReviewer = vi.fn( - () => - new Promise((resolve) => { - resolveReview = resolve; - }), - ); + const autoReviewer = vi.fn(() => new Promise(() => {})); resolveExecHostApprovalContextMock.mockReturnValue({ approvals: { allowlist: [], file: { version: 1, agents: {} } }, hostSecurity: "allowlist", @@ -1305,7 +1299,6 @@ describe("executeNodeHostCommand", () => { const gatewayCallsBeforeResolution = callGatewayToolMock.mock.calls.length; abortController.abort(new Error("cancelled during review")); - resolveReview?.({ decision: "allow-once", risk: "low", rationale: "allowed" }); await expect(result).rejects.toThrow("cancelled during review"); expect(callGatewayToolMock.mock.calls).toHaveLength(gatewayCallsBeforeResolution); diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts index 97d8211f6ad1..960fa47c2a75 100644 --- a/src/agents/bash-tools.exec-host-node.ts +++ b/src/agents/bash-tools.exec-host-node.ts @@ -38,6 +38,7 @@ import { normalizeNotifyOutput, } from "./bash-tools.exec-runtime.js"; import type { ExecToolDetails } from "./bash-tools.exec-types.js"; +import { abortable } from "./embedded-agent-runner/run/abortable.js"; import type { AgentToolResult } from "./runtime/index.js"; import { callGatewayTool } from "./tools/gateway.js"; @@ -376,30 +377,36 @@ export async function executeNodeHostCommand( !requiresSecurityAuditSuppressionApproval ) { const reviewer = params.autoReviewer ?? defaultExecAutoReviewer; - const decision = await reviewer({ - command: prepared.rawCommand, - argv: autoReviewArgv, - cwd: prepared.cwd, - envKeys: Object.keys(params.requestedEnv ?? {}).toSorted(), - host: "node", - reason: resolveNodeAutoReviewReason({ - inlineEvalHit, - hostSecurity, - analysisOk, - allowlistSatisfied, - durableApprovalSatisfied, + const pendingDecision = Promise.resolve( + reviewer({ + command: prepared.rawCommand, + argv: autoReviewArgv, + cwd: prepared.cwd, + envKeys: Object.keys(params.requestedEnv ?? {}).toSorted(), + host: "node", + reason: resolveNodeAutoReviewReason({ + inlineEvalHit, + hostSecurity, + analysisOk, + allowlistSatisfied, + durableApprovalSatisfied, + }), + analysis: { + parsed: analysisOk, + allowlistMatched: allowlistSatisfied, + durableApprovalMatched: durableApprovalSatisfied, + inlineEval: inlineEvalHit !== null, + }, + agent: { + id: prepared.agentId, + sessionKey: prepared.sessionKey, + }, }), - analysis: { - parsed: analysisOk, - allowlistMatched: allowlistSatisfied, - durableApprovalMatched: durableApprovalSatisfied, - inlineEval: inlineEvalHit !== null, - }, - agent: { - id: prepared.agentId, - sessionKey: prepared.sessionKey, - }, - }); + ); + // An injected reviewer cannot keep a cancelled node invocation or approval alive. + const decision = params.signal + ? await abortable(params.signal, pendingDecision) + : await pendingDecision; params.signal?.throwIfAborted(); const autoReviewAllowed = decision.decision === "allow-once" && decision.risk === "low"; if (autoReviewAllowed) { diff --git a/src/agents/bash-tools.exec-run.ts b/src/agents/bash-tools.exec-run.ts index c60f3af06d03..6a118a1c23c4 100644 --- a/src/agents/bash-tools.exec-run.ts +++ b/src/agents/bash-tools.exec-run.ts @@ -140,14 +140,6 @@ export function createExecTool( agentId, resolveHostForParams, }); - const autoReviewer = - defaults?.autoReviewer ?? - createModelExecAutoReviewer({ - cfg: defaults?.config, - agentId, - reviewer: resolveExecReviewerDefaults({ defaults, agentId }), - }); - return { name: "exec", label: "exec", @@ -160,6 +152,15 @@ export function createExecTool( finalizeBeforeToolCallParams: requestPreparation.finalizeBeforeToolCallParams, execute: async (toolCallId, args, signal, onUpdate) => { signal?.throwIfAborted(); + // Review cancellation belongs to this execution, never another call on the shared tool. + const autoReviewer = + defaults?.autoReviewer ?? + createModelExecAutoReviewer({ + cfg: defaults?.config, + agentId, + reviewer: resolveExecReviewerDefaults({ defaults, agentId }), + signal, + }); let params = requestPreparation.normalizeParams(args); const resolveExecEnvPrepared = requestPreparation.isResolveExecEnvPrepared( args as ExecToolArgs, diff --git a/src/agents/exec-auto-review.stress.test.ts b/src/agents/exec-auto-review.stress.test.ts index 5fe328ff122c..1bd06967c429 100644 --- a/src/agents/exec-auto-review.stress.test.ts +++ b/src/agents/exec-auto-review.stress.test.ts @@ -33,6 +33,7 @@ type StressCompletionResult = { function createStressReviewer(params: { complete: (request: StressCompletionRequest) => Promise; + signal?: AbortSignal; timeoutMs?: number; }) { const prepare = vi.fn(async () => ({ @@ -43,6 +44,7 @@ function createStressReviewer(params: { const complete = vi.fn(params.complete); const reviewer = createModelExecAutoReviewer({ cfg: {}, + signal: params.signal, ...(params.timeoutMs === undefined ? {} : { reviewer: { timeoutMs: params.timeoutMs } }), deps: { prepareSimpleCompletionModelForAgent: @@ -85,7 +87,7 @@ function chooseSeeded(random: () => number, values: readonly T[]): T { return selected; } -describe.runIf(process.platform !== "win32")("exec auto-review shell stress", () => { +describe("exec auto-review shell stress", () => { it("keeps mutable PowerShell script entry points outside auto-review", () => { const positionalScript = ["pwsh", "-NoProfile", "./safe.ps1"]; const explicitScript = ["pwsh", "-NoProfile", "-File", "./safe.ps1"]; @@ -388,7 +390,7 @@ describe.runIf(process.platform !== "win32")("exec auto-review shell stress", () } }); - it.each([ + it.runIf(process.platform !== "win32").each([ ["bash combined login", "bash -lc 'echo startup'"], ["bash reversed login flags", "bash -cl 'echo startup'"], ["bash separate login flags", "bash -l -c 'echo startup'"], @@ -596,19 +598,21 @@ describe.runIf(process.platform !== "win32")("exec auto-review shell stress", () } }); - it.each([ - "node --version", - "git status", - "printf safe", - "cmd /d /c echo safe", - "cmd.exe /d /s /c echo safe", - "CMD.EXE /D /S /C echo safe", - "cmd /d /s /q /c echo safe", - "cmd /d /e:on /f:off /v:on /t:0f /c echo safe", - "pwsh -NoProfile -Command 'Write-Output safe'", - "pwsh -nop -c 'Write-Output safe'", - "pwsh /NoProfile /c 'Write-Output safe'", - ])("preserves ordinary reviewable command: %s", async (command) => { + it + .runIf(process.platform !== "win32") + .each([ + "node --version", + "git status", + "printf safe", + "cmd /d /c echo safe", + "cmd.exe /d /s /c echo safe", + "CMD.EXE /D /S /C echo safe", + "cmd /d /s /q /c echo safe", + "cmd /d /e:on /f:off /v:on /t:0f /c echo safe", + "pwsh -NoProfile -Command 'Write-Output safe'", + "pwsh -nop -c 'Write-Output safe'", + "pwsh /NoProfile /c 'Write-Output safe'", + ])("preserves ordinary reviewable command: %s", async (command) => { for (const host of ["gateway", "node", "codex-app-server"] as const) { await expect( buildExecAutoReviewInputForShellCommand({ @@ -722,6 +726,50 @@ describe("exec auto-review adversarial model-response stress", () => { }); describe("exec auto-review concurrency stress", () => { + it("cancels 64 concurrent provider reviews without sharing execution signals", async () => { + const controllers = Array.from({ length: 64 }, () => new AbortController()); + const observedSignals: AbortSignal[] = []; + const requests = controllers.map((controller, index) => { + const { reviewer } = createStressReviewer({ + signal: controller.signal, + complete: async (request) => { + const providerSignal = request.options.signal; + if (!providerSignal) { + throw new Error("review completion must receive an abort signal"); + } + observedSignals.push(providerSignal); + return await new Promise((_resolve, reject) => { + providerSignal.addEventListener("abort", () => reject(new Error("provider aborted")), { + once: true, + }); + }); + }, + }); + return Promise.resolve( + reviewer({ + ...baselineInput, + command: `git status --cancel-case=${index}`, + argv: ["git", "status", `--cancel-case=${index}`], + }), + ); + }); + const settled = Promise.allSettled(requests); + await vi.waitFor(() => expect(observedSignals).toHaveLength(controllers.length)); + + for (const [index, controller] of controllers.entries()) { + controller.abort(new Error(`cancelled review ${index}`)); + } + + const results = await settled; + for (const [index, result] of results.entries()) { + expect(result.status, `cancelled review ${index}`).toBe("rejected"); + if (result.status === "rejected") { + expect(result.reason).toMatchObject({ message: `cancelled review ${index}` }); + } + } + expect(observedSignals.every((signal) => signal.aborted)).toBe(true); + }); + it("keeps 256 concurrent approvals independently bound and single-use", async () => { const { reviewer, prepare, complete } = createStressReviewer({ complete: async () => modelResponse("allow", "low"), diff --git a/src/agents/exec-auto-reviewer.test.ts b/src/agents/exec-auto-reviewer.test.ts index 085025e796c3..adc4fab79125 100644 --- a/src/agents/exec-auto-reviewer.test.ts +++ b/src/agents/exec-auto-reviewer.test.ts @@ -523,6 +523,59 @@ describe("createModelExecAutoReviewer", () => { } }); + it("cancels pending model preparation with the execution", async () => { + const controller = new AbortController(); + const prepare = vi.fn(() => new Promise(() => {})); + const reviewer = createModelExecAutoReviewer({ + cfg: {}, + signal: controller.signal, + deps: { + prepareSimpleCompletionModelForAgent: + prepare as unknown as typeof import("./simple-completion-runtime.js").prepareSimpleCompletionModelForAgent, + }, + }); + + const result = reviewer(input); + await vi.waitFor(() => expect(prepare).toHaveBeenCalledTimes(1)); + controller.abort(new Error("execution cancelled during reviewer preparation")); + + await expect(result).rejects.toThrow("execution cancelled during reviewer preparation"); + }); + + it("aborts a pending provider review when its execution is cancelled", async () => { + const controller = new AbortController(); + let providerSignal: AbortSignal | undefined; + const complete = vi.fn( + (request: { options: { signal?: AbortSignal } }) => + new Promise((_resolve, reject) => { + providerSignal = request.options.signal; + providerSignal?.addEventListener("abort", () => reject(new Error("provider aborted")), { + once: true, + }); + }), + ); + const reviewer = createModelExecAutoReviewer({ + cfg: {}, + signal: controller.signal, + deps: { + prepareSimpleCompletionModelForAgent: vi.fn(async () => ({ + selection: { provider: "openrouter", modelId: "reviewer", agentDir: "/agent" }, + model: { provider: "openrouter", id: "reviewer", api: "openai" as const }, + auth: { apiKey: "redacted", mode: "env" as const }, + })) as unknown as typeof import("./simple-completion-runtime.js").prepareSimpleCompletionModelForAgent, + completeWithPreparedSimpleCompletionModel: + complete as unknown as typeof import("./simple-completion-runtime.js").completeWithPreparedSimpleCompletionModel, + }, + }); + + const result = reviewer(input); + await vi.waitFor(() => expect(complete).toHaveBeenCalledTimes(1)); + controller.abort(new Error("execution cancelled during provider review")); + + await expect(result).rejects.toThrow("execution cancelled during provider review"); + expect(providerSignal?.aborted).toBe(true); + }); + it("caps oversized reviewer timeouts before scheduling timers", async () => { vi.useFakeTimers(); try { diff --git a/src/agents/exec-auto-reviewer.ts b/src/agents/exec-auto-reviewer.ts index 56a87e347a63..be4640da4681 100644 --- a/src/agents/exec-auto-reviewer.ts +++ b/src/agents/exec-auto-reviewer.ts @@ -18,6 +18,7 @@ import { type ExecAutoReviewInput, type ExecAutoReviewer, } from "../infra/exec-auto-review.js"; +import { abortable } from "./embedded-agent-runner/run/abortable.js"; import { DEFAULT_EXEC_REVIEWER_SYSTEM_PROMPT } from "./exec-auto-reviewer.prompt.js"; import { completeWithPreparedSimpleCompletionModel, @@ -327,6 +328,7 @@ async function raceWithReviewerTimeout( params: { timeoutMs: number; onTimeout?: () => void; + signal?: AbortSignal; }, ): Promise { let timer: ReturnType | undefined; @@ -337,7 +339,8 @@ async function raceWithReviewerTimeout( }, params.timeoutMs); }); try { - return await Promise.race([promise, timeout]); + const pending = Promise.race([promise, timeout]); + return params.signal ? await abortable(params.signal, pending) : await pending; } finally { if (timer) { clearTimeout(timer); @@ -351,6 +354,7 @@ export function createModelExecAutoReviewer(params: { agentId?: string; reviewer?: ExecReviewerConfig; deps?: ExecReviewerDeps; + signal?: AbortSignal; }): ExecAutoReviewer { const cfg = params.cfg; const agentId = params.agentId ?? "main"; @@ -367,6 +371,7 @@ export function createModelExecAutoReviewer(params: { return async (input) => { let completionController: AbortController | undefined; try { + params.signal?.throwIfAborted(); if (hasReviewerDirective(input)) { return { decision: "ask", @@ -381,7 +386,7 @@ export function createModelExecAutoReviewer(params: { modelRef, allowMissingApiKeyModes: ["aws-sdk"], }), - { timeoutMs }, + { timeoutMs, signal: params.signal }, ); if (prepared === EXEC_REVIEWER_TIMEOUT) { return buildReviewerTimeoutDecision(timeoutMs); @@ -413,11 +418,14 @@ export function createModelExecAutoReviewer(params: { options: { maxTokens: EXEC_REVIEWER_MAX_TOKENS, temperature: 0, - signal: completionController.signal, + signal: params.signal + ? AbortSignal.any([completionController.signal, params.signal]) + : completionController.signal, }, }), { timeoutMs, + signal: params.signal, // Abort the provider request after the local timeout wins the race. onTimeout: () => completionController?.abort(), }, @@ -435,6 +443,7 @@ export function createModelExecAutoReviewer(params: { } return parseExecAutoReviewResponse(extractTextContent(result)); } catch (err) { + params.signal?.throwIfAborted(); if (completionController?.signal.aborted) { return buildReviewerTimeoutDecision(timeoutMs); } diff --git a/test/scripts/pr-ci-sweeper.test.ts b/test/scripts/pr-ci-sweeper.test.ts index e256dc0bd532..2059d74f767d 100644 --- a/test/scripts/pr-ci-sweeper.test.ts +++ b/test/scripts/pr-ci-sweeper.test.ts @@ -294,11 +294,12 @@ function fakeGithub(options: { >; checksByRef?: Record; workflowRunsById?: Record; - pullsGetByNumber?: Record>; + pullsGetByNumber?: Record | Array>>; events?: Array>; pageSize?: number; }) { const calls: FakeCall[] = []; + const pullsGetCallCounts = new Map(); const record = (method: string, args: Record) => { calls.push({ method, args }); }; @@ -351,9 +352,13 @@ function fakeGithub(options: { list: { endpointName: "pulls.list" }, get: (args: Record) => { record("pulls.get", args); - const match = - options.pullsGetByNumber?.[args.pull_number as number] ?? - options.prs.find((entry) => entry.number === args.pull_number); + const pullNumber = args.pull_number as number; + const configured = options.pullsGetByNumber?.[pullNumber]; + const callIndex = pullsGetCallCounts.get(pullNumber) ?? 0; + pullsGetCallCounts.set(pullNumber, callIndex + 1); + const match = Array.isArray(configured) + ? configured[Math.min(callIndex, configured.length - 1)] + : (configured ?? options.prs.find((entry) => entry.number === pullNumber)); return Promise.resolve({ data: match }); }, update: (args: Record) => { @@ -544,6 +549,54 @@ describe("runPrCiSweeper", () => { ).toEqual([]); }); + it("does not spend the re-fire budget on PRs that change during revalidation", async () => { + const dropped = Array.from({ length: 11 }, (_, index) => ({ + ...pr(), + number: 200 + index, + state: "open", + head: { sha: index.toString(16).padStart(2, "0").repeat(20) }, + })); + const pullsGetByNumber = Object.fromEntries( + dropped + .slice(0, 10) + .map((candidate) => [ + candidate.number, + [candidate, { ...candidate, head: { sha: "f".repeat(40) } }], + ]), + ); + const { github, calls } = fakeGithub({ prs: dropped, runsBySha: {}, pullsGetByNumber }); + const { core: loggedCore, logs } = recordingCore(); + + const results = await runPrCiSweeper({ + github: github as never, + context: context as never, + core: loggedCore as never, + now: NOW, + }); + + expect(results.slice(0, 10)).toEqual( + dropped.slice(0, 10).map((candidate) => ({ + number: candidate.number, + sha: candidate.head.sha.slice(0, 12), + action: "skip", + reason: "changed-during-sweep", + })), + ); + expect(results.at(-1)).toEqual({ + number: 210, + sha: "0a".repeat(6), + action: "refire", + reason: "ci-run-missing", + }); + expect(calls.filter((call) => call.method === "pulls.update").map((call) => call.args)).toEqual( + [ + { owner: "openclaw", repo: "openclaw", pull_number: 210, state: "closed" }, + { owner: "openclaw", repo: "openclaw", pull_number: 210, state: "open" }, + ], + ); + expect(logs.at(-1)).toContain("1 re-fire"); + }); + it("stops listing pages once creation dates cross the lookback", async () => { const recent = { ...pr(), number: 30, state: "open", head: { sha: "7".repeat(40) } }; const oldA = { diff --git a/test/scripts/pr-merge.test.ts b/test/scripts/pr-merge.test.ts index 63e98f8a9c2e..b2ae527265ce 100644 --- a/test/scripts/pr-merge.test.ts +++ b/test/scripts/pr-merge.test.ts @@ -13,10 +13,12 @@ const describePosix = process.platform === "win32" ? describe.skip : describe; type MergeScenario = { auto?: boolean; autoResult?: "enabled" | "inconclusive" | "unavailable"; - checks?: "fail" | "green"; + checks?: "fail" | "green" | "pending"; existingAutoMethod?: "" | "MERGE" | "REBASE" | "SQUASH"; mergeStateStatus?: string; mergeable?: string; + recommendation?: "ready" | "needs_work"; + reviewArtifacts?: "valid" | "invalid"; }; function runMerge(scenario: MergeScenario = {}) { @@ -59,13 +61,27 @@ function runMerge(scenario: MergeScenario = {}) { const checks = scenario.checks === "fail" ? [{ name: "CI", bucket: "fail", state: "FAILURE" }] - : [{ name: "CI", bucket: "pass", state: "SUCCESS" }]; + : scenario.checks === "pending" + ? [{ name: "CI", bucket: "pending", state: "IN_PROGRESS" }] + : [{ name: "CI", bucket: "pass", state: "SUCCESS" }]; const shell = ` set -euo pipefail source "$OPENCLAW_TEST_MERGE_SCRIPT" enter_worktree() { :; } require_artifact() { :; } +validate_review_artifact_data() { + if [ "$OPENCLAW_TEST_REVIEW_ARTIFACTS" != "valid" ]; then + echo 'review artifact validation failed' >&2 + return 1 + fi +} +require_ready_review_recommendation() { + if [ "$OPENCLAW_TEST_REVIEW_RECOMMENDATION" != "ready" ]; then + echo 'review recommendation is not ready' >&2 + return 1 + fi +} verify_prep_branch_matches_prepared_head() { :; } mark_pr_operation_side_effects_started() { :; } mainline_drift_requires_sync() { return 1; } @@ -90,7 +106,10 @@ gh() { case "$1 $2" in "pr checks") case " $* " in - *" --json "*) printf '%s\\n' "$OPENCLAW_TEST_CHECKS_JSON" ;; + *" --json "*) + printf '%s\\n' "$OPENCLAW_TEST_CHECKS_JSON" + return "$OPENCLAW_TEST_CHECKS_EXIT_STATUS" + ;; esac ;; "pr view") @@ -161,6 +180,7 @@ merge_run 123 "$OPENCLAW_TEST_AUTO_REQUESTED" OPENCLAW_TEST_AUTO_REQUESTED: scenario.auto ? "true" : "false", OPENCLAW_TEST_AUTO_RESULT: scenario.autoResult ?? "enabled", OPENCLAW_TEST_AUTO_STATE: autoState, + OPENCLAW_TEST_CHECKS_EXIT_STATUS: scenario.checks === "pending" ? "8" : "0", OPENCLAW_TEST_CHECKS_JSON: JSON.stringify(checks), OPENCLAW_TEST_DISABLED_AUTO_META: disabledAutoMeta, OPENCLAW_TEST_GH_CALLS: calls, @@ -169,6 +189,8 @@ merge_run 123 "$OPENCLAW_TEST_AUTO_REQUESTED" OPENCLAW_TEST_MERGE_STATE_STATUS: scenario.mergeStateStatus ?? "BEHIND", OPENCLAW_TEST_POST_AUTO_META: postAutoMeta, OPENCLAW_TEST_PRE_AUTO_META: preAutoMeta, + OPENCLAW_TEST_REVIEW_ARTIFACTS: scenario.reviewArtifacts ?? "valid", + OPENCLAW_TEST_REVIEW_RECOMMENDATION: scenario.recommendation ?? "ready", OPENCLAW_TEST_ROOT: root, }, }); @@ -179,6 +201,22 @@ merge_run 123 "$OPENCLAW_TEST_AUTO_REQUESTED" } describePosix("scripts/pr merge-run", () => { + it("refuses to merge when review artifact validation fails", () => { + const result = runMerge({ reviewArtifacts: "invalid" }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("review artifact validation failed"); + expect(result.calls).not.toContain("pr merge"); + }); + + it("refuses to merge when the review recommendation is not ready", () => { + const result = runMerge({ recommendation: "needs_work" }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("review recommendation is not ready"); + expect(result.calls).not.toContain("pr merge"); + }); + it("does not enable auto-merge when exact-head required CI is failing", () => { const result = runMerge({ auto: true, checks: "fail" }); @@ -187,6 +225,15 @@ describePosix("scripts/pr merge-run", () => { expect(result.calls).not.toContain("pr merge"); }); + it("does not mistake pending required checks for a GitHub API failure", () => { + const result = runMerge({ auto: true, checks: "pending" }); + + expect(result.status).toBe(1); + expect(result.stdout).toContain("Required checks are still pending."); + expect(result.stderr).not.toContain("unable to verify the required GitHub checks"); + expect(result.calls).not.toContain("pr merge"); + }); + it("fails a conflicting PR without attempting auto-merge", () => { const result = runMerge({ auto: true, diff --git a/test/scripts/pr-prepare-gates.test.ts b/test/scripts/pr-prepare-gates.test.ts index 2723350fe206..f7c54610f371 100644 --- a/test/scripts/pr-prepare-gates.test.ts +++ b/test/scripts/pr-prepare-gates.test.ts @@ -535,6 +535,50 @@ describe("lease-retry gate stamp refresh", () => { }); }); +describe("prepare review readiness", () => { + it("rejects invalid review artifacts before any preparation side effects", () => { + const repoDir = makeTempDir("openclaw-pr-prepare-invalid-review-"); + mkdirSync(join(repoDir, ".local")); + const result = runGatesBash( + [ + "review_validate_artifacts() { echo 'invalid review artifacts'; return 1; }", + "require_ready_review_recommendation() { touch .local/readiness-called; }", + "mark_pr_operation_side_effects_started() { touch .local/side-effects; }", + "enter_worktree() { touch .local/worktree-entered; }", + "prepare_init 4242", + ].join("\n"), + { cwd: repoDir, sourcePrepareCore: true }, + ); + + expect(result.status).toBe(1); + expect(result.stdout).toContain("invalid review artifacts"); + expect(existsSync(join(repoDir, ".local", "readiness-called"))).toBe(false); + expect(existsSync(join(repoDir, ".local", "side-effects"))).toBe(false); + expect(existsSync(join(repoDir, ".local", "worktree-entered"))).toBe(false); + }); + + it("rejects a non-ready review before taking the operation lock past validation", () => { + const repoDir = makeTempDir("openclaw-pr-prepare-not-ready-"); + mkdirSync(join(repoDir, ".local")); + const result = runGatesBash( + [ + "review_validate_artifacts() { touch .local/review-validated; }", + "require_ready_review_recommendation() { echo 'review is not ready'; return 1; }", + "mark_pr_operation_side_effects_started() { touch .local/side-effects; }", + "enter_worktree() { touch .local/worktree-entered; }", + "prepare_init 4242", + ].join("\n"), + { cwd: repoDir, sourcePrepareCore: true }, + ); + + expect(result.status).toBe(1); + expect(result.stdout).toContain("review is not ready"); + expect(existsSync(join(repoDir, ".local", "review-validated"))).toBe(true); + expect(existsSync(join(repoDir, ".local", "side-effects"))).toBe(false); + expect(existsSync(join(repoDir, ".local", "worktree-entered"))).toBe(false); + }); +}); + describe("prepare sync-head transitions", () => { it("publishes only appended fixups when main advances", () => { const repoDir = makeSyncRepo({ needsRebase: true }); diff --git a/test/scripts/pr-review-artifact-validation.test.ts b/test/scripts/pr-review-artifact-validation.test.ts index 1ebf541ff8f2..cb31d04bb9d9 100644 --- a/test/scripts/pr-review-artifact-validation.test.ts +++ b/test/scripts/pr-review-artifact-validation.test.ts @@ -7,12 +7,19 @@ import { useAutoCleanupTempDirTracker } from "../helpers/temp-dir.js"; const tempDirs = useAutoCleanupTempDirTracker(afterEach); const reviewScript = join(process.cwd(), "scripts/pr-lib/review.sh"); const reviewArtifactsScript = join(process.cwd(), "scripts/pr-lib/review-artifacts.mjs"); +const mergeScript = join(process.cwd(), "scripts/pr-lib/merge.sh"); const describePosix = process.platform === "win32" ? describe.skip : describe; function validReview() { return { recommendation: "NEEDS WORK", - findings: [], + findings: [] as Array<{ + id: string; + title: string; + area: string; + fix: string; + severity: "BLOCKER" | "IMPORTANT" | "NIT"; + }>, nitSweep: { performed: true, status: "none", @@ -41,7 +48,22 @@ function validReview() { }; } -function runValidation(review: ReturnType) { +function validReadyReview() { + const review = validReview(); + review.recommendation = "READY FOR /prepare-pr"; + review.issueValidation.status = "valid"; + return review; +} + +function runValidation( + review: ReturnType, + options: { + files?: string[]; + guardFailure?: boolean; + mode?: "pr" | "main"; + orList?: boolean; + } = {}, +) { const fixtureRoot = tempDirs.make("openclaw-pr-review-validation-"); const localDir = join(fixtureRoot, ".local"); mkdirSync(localDir); @@ -51,7 +73,10 @@ function runValidation(review: ReturnType) { ["A)", "B)", "C)", "D)", "E)", "F)", "G)", "H)", "I)", "J)"].join("\n"), ); writeFileSync(join(localDir, "pr-meta.env"), "PR_URL=https://example.invalid/pr/42\n"); - writeFileSync(join(localDir, "pr-meta.json"), '{"files":[]}\n'); + writeFileSync( + join(localDir, "pr-meta.json"), + `${JSON.stringify({ files: (options.files ?? []).map((path) => ({ path })) })}\n`, + ); return spawnSync( "bash", @@ -63,9 +88,11 @@ function runValidation(review: ReturnType) { 'fixture_root="$2"', 'enter_worktree() { cd "$fixture_root"; }', 'require_artifact() { [ -s "$1" ]; }', - "review_guard() { :; }", + options.guardFailure + ? "review_guard() { REVIEW_MODE=pr; echo 'review head guard failed'; return 1; }" + : `review_guard() { REVIEW_MODE=${options.mode ?? "pr"}; }`, "print_review_stdout_summary() { :; }", - "review_validate_artifacts 42", + options.orList ? "review_validate_artifacts 42 || exit 1" : "review_validate_artifacts 42", ].join("\n"), "pr-review-artifact-validation", reviewScript, @@ -75,6 +102,47 @@ function runValidation(review: ReturnType) { ); } +function runMergeVerification(checks: "api-error" | "invalid-json" | "no-required" | "pending") { + const fixtureRoot = tempDirs.make("openclaw-pr-merge-verification-"); + const localDir = join(fixtureRoot, ".local"); + const head = "a".repeat(40); + mkdirSync(localDir); + writeFileSync(join(localDir, "prep.env"), `PREP_HEAD_SHA=${head}\n`); + + const checksResponse = + checks === "api-error" + ? "echo 'GitHub API unavailable' >&2; return 1" + : checks === "no-required" + ? "echo \"no required checks reported on the 'review-branch' branch\" >&2; return 1" + : checks === "pending" + ? `printf '%s\\n' '[{"name":"CI","bucket":"pending","state":"IN_PROGRESS"}]'; return 8` + : "printf '%s\\n' 'not valid JSON'"; + + return spawnSync( + "bash", + [ + "-c", + [ + "set -euo pipefail", + 'source "$1"', + 'fixture_root="$2"', + 'enter_worktree() { cd "$fixture_root"; }', + 'require_artifact() { [ -s "$1" ]; }', + "verify_prep_branch_matches_prepared_head() { :; }", + `pr_meta_json() { printf '%s\\n' '{"isDraft":false,"headRefOid":"${head}"}'; }`, + "mark_pr_operation_side_effects_started() { :; }", + "git() { :; }", + `gh() { case "$*" in *"--json name,bucket,state"*) ${checksResponse};; *) return 0;; esac; }`, + "merge_verify 42", + ].join("\n"), + "pr-merge-verification", + mergeScript, + fixtureRoot, + ], + { encoding: "utf8" }, + ); +} + describePosix("scripts/pr review artifact validation", () => { it("accepts a valid review artifact", () => { const result = runValidation(validReview()); @@ -83,6 +151,146 @@ describePosix("scripts/pr review artifact validation", () => { expect(result.stdout).toContain("review artifacts validated"); }); + it("rejects validation from main-baseline mode", () => { + const result = runValidation(validReview(), { mode: "main" }); + + expect(result.status).toBe(1); + expect(result.stdout).toContain( + "Review artifact validation requires the reviewed PR head, not main-baseline mode.", + ); + }); + + it("preserves head-guard failures when preparation calls validation in an OR-list", () => { + const result = runValidation(validReview(), { guardFailure: true, orList: true }); + + expect(result.status).toBe(1); + expect(result.stdout).toContain("review head guard failed"); + expect(result.stdout).not.toContain("review artifacts validated"); + }); + + it.each(["BLOCKER", "IMPORTANT"] as const)( + "rejects a ready review containing a %s finding", + (severity) => { + const review = validReadyReview(); + review.findings.push({ + id: "review-finding", + title: "Actionable review finding", + area: "runtime", + fix: "Resolve the finding before preparing the PR.", + severity, + }); + + const result = runValidation(review); + + expect(result.status).toBe(1); + expect(result.stdout).toContain( + "READY FOR /prepare-pr cannot include BLOCKER or IMPORTANT findings", + ); + }, + ); + + it("keeps non-ready findings and failed proof valid for review triage", () => { + const review = validReview(); + review.findings.push({ + id: "review-finding", + title: "Actionable review finding", + area: "runtime", + fix: "Resolve the finding before preparing the PR.", + severity: "IMPORTANT", + }); + review.tests.result = "fail"; + + const result = runValidation(review); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + }); + + it("rejects a ready review with failing proof", () => { + const review = validReadyReview(); + review.tests.result = "fail"; + + const result = runValidation(review); + + expect(result.status).toBe(1); + expect(result.stdout).toContain("READY FOR /prepare-pr cannot include failing tests"); + }); + + it("permits documentation-only ready reviews without runtime tests", () => { + const review = validReadyReview(); + review.tests.result = "not_run"; + + const result = runValidation(review, { files: ["docs/reference/example.md"] }); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + }); + + it.each([ + "packages/normalization-core/src/string-normalization.ts", + "packages/gateway-protocol/src/schema/approvals.ts", + "ui/src/app.ts", + ])("requires behavioral review for core runtime path %s", (path) => { + const result = runValidation(validReview(), { files: [path] }); + + expect(result.status).toBe(1); + expect(result.stdout).toContain( + "runtime file changes require behavioralSweep.status=pass|needs_work", + ); + expect(result.stdout).toContain("runtime file changes require at least one branch entry"); + }); + + it("requires passing runtime proof for a ready review", () => { + const review = validReadyReview(); + review.behavioralSweep.status = "pass"; + review.behavioralSweep.branches.push({ + path: "ui/src/app.ts", + decision: "verified", + outcome: "Behavior remains correct.", + }); + review.tests.result = "not_run"; + + const result = runValidation(review, { files: ["ui/src/app.ts"] }); + + expect(result.status).toBe(1); + expect(result.stdout).toContain( + "READY FOR /prepare-pr on runtime changes requires passing tests", + ); + }); + + it("rejects merge verification when GitHub cannot verify required checks", () => { + const result = runMergeVerification("api-error"); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("unable to verify the required GitHub checks"); + expect(result.stderr).toContain("GitHub API unavailable"); + expect(result.stdout).not.toContain("merge-verify passed"); + expect(result.stdout).not.toContain("No required checks configured"); + }); + + it("preserves GitHub CLI behavior when a branch has no required checks", () => { + const result = runMergeVerification("no-required"); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain("No required checks configured for this PR."); + expect(result.stdout).toContain("merge-verify passed for PR #42"); + }); + + it("preserves GitHub CLI pending-check evidence from exit status eight", () => { + const result = runMergeVerification("pending"); + + expect(result.status).toBe(1); + expect(result.stdout).toContain("Required checks are still pending."); + expect(result.stderr).not.toContain("unable to verify the required GitHub checks"); + expect(result.stdout).not.toContain("merge-verify passed"); + }); + + it("rejects merge verification when GitHub returns malformed check evidence", () => { + const result = runMergeVerification("invalid-json"); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("GitHub returned invalid required-check evidence"); + expect(result.stdout).not.toContain("merge-verify passed"); + }); + it("reports the required branch entry shape without a raw jq error", () => { const review = validReview(); review.behavioralSweep.branches = ["src/example.ts"];