vultr/openclaw
1
0
Fork 0
mirror of https://github.com/openclaw/openclaw.git synced 2026-08-02 08:31:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: make auto-review and PR triage fail safely (#114745)

Browse Source 
* fix: make auto-review and PR triage fail safely

* test: cover fail-closed PR merge review gates

* fix: preserve GitHub pending-check review semantics
This commit is contained in:
Peter Steinberger
2026-07-27 17:11:28 -04:00
committed by GitHub
parent 1c0ccdddcd
commit 6f7edb3695
17 changed files with 685 additions and 126 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
scripts/github/pr-ci-sweeper.mjs
View File

@@ -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),
);

41
scripts/pr-lib/merge.sh
View File

@@ -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

7
scripts/pr-lib/prepare-core.sh
View File

@@ -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=$(

27
scripts/pr-lib/review-artifacts.mjs
View File

@@ -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",

37
scripts/pr-lib/review.sh
View File

@@ -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

9
src/agents/bash-tools.exec-host-gateway.test.ts
View File

@@ -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<ReturnType<ExecAutoReviewer>>) => void) | undefined;
const autoReviewer = vi.fn<ExecAutoReviewer>(
() =>
new Promise((resolve) => {
resolveReview = resolve;
}),
);
const autoReviewer = vi.fn<ExecAutoReviewer>(() => 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();

61
src/agents/bash-tools.exec-host-gateway.ts
View File

@@ -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" &&

9
src/agents/bash-tools.exec-host-node.test.ts
View File

@@ -1268,13 +1268,7 @@ describe("executeNodeHostCommand", () => {
});
it("does not invoke the node after cancellation wins during auto-review", async () => {
let resolveReview: ((decision: Awaited<ReturnType<ExecAutoReviewer>>) => void) | undefined;
const autoReviewer = vi.fn<ExecAutoReviewer>(
() =>
new Promise((resolve) => {
resolveReview = resolve;
}),
);
const autoReviewer = vi.fn<ExecAutoReviewer>(() => 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);

53
src/agents/bash-tools.exec-host-node.ts
View File

@@ -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) {

17
src/agents/bash-tools.exec-run.ts
View File

@@ -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,

78
src/agents/exec-auto-review.stress.test.ts
View File

@@ -33,6 +33,7 @@ type StressCompletionResult = {
function createStressReviewer(params: {
complete: (request: StressCompletionRequest) => Promise<StressCompletionResult>;
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<T>(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<StressCompletionResult>((_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"),

53
src/agents/exec-auto-reviewer.test.ts
View File

@@ -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<never>(() => {}));
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<never>((_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 {

15
src/agents/exec-auto-reviewer.ts
View File

@@ -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<T>(
params: {
timeoutMs: number;
onTimeout?: () => void;
signal?: AbortSignal;
},
): Promise<T | typeof EXEC_REVIEWER_TIMEOUT> {
let timer: ReturnType<typeof setTimeout> | undefined;
@@ -337,7 +339,8 @@ async function raceWithReviewerTimeout<T>(
}, 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);
}

61
test/scripts/pr-ci-sweeper.test.ts
View File

@@ -294,11 +294,12 @@ function fakeGithub(options: {
>;
checksByRef?: Record<string, FakeCheckRun[]>;
workflowRunsById?: Record<number, FakeWorkflowRun>;
pullsGetByNumber?: Record<number, Record<string, unknown>>;
pullsGetByNumber?: Record<number, Record<string, unknown> | Array<Record<string, unknown>>>;
events?: Array<Record<string, unknown>>;
pageSize?: number;
}) {
const calls: FakeCall[] = [];
const pullsGetCallCounts = new Map<number, number>();
const record = (method: string, args: Record<string, unknown>) => {
calls.push({ method, args });
};
@@ -351,9 +352,13 @@ function fakeGithub(options: {
list: { endpointName: "pulls.list" },
get: (args: Record<string, unknown>) => {
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<string, unknown>) => {
@@ -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 = {

53
test/scripts/pr-merge.test.ts
View File

@@ -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,

44
test/scripts/pr-prepare-gates.test.ts
View File

@@ -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 });

218
test/scripts/pr-review-artifact-validation.test.ts
View File

@@ -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<typeof validReview>) {
function validReadyReview() {
const review = validReview();
review.recommendation = "READY FOR /prepare-pr";
review.issueValidation.status = "valid";
return review;
}
function runValidation(
review: ReturnType<typeof validReview>,
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<typeof validReview>) {
["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<typeof validReview>) {
'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<typeof validReview>) {
);
}
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"];