From b4d386c779b89f64777d76b42bc4c7e88c89f706 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 31 Jul 2026 20:17:58 +0200 Subject: [PATCH 1/3] fix(review): materialize exact PR source --- .github/workflows/sweep.yml | 62 +++++++++++++++++++++++++++----- test/sweep-workflow.test.ts | 72 +++++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 8 deletions(-) diff --git a/.github/workflows/sweep.yml b/.github/workflows/sweep.yml index 4b2ebf404f..334166ce52 100644 --- a/.github/workflows/sweep.yml +++ b/.github/workflows/sweep.yml @@ -857,7 +857,12 @@ jobs: ${{ steps.target.outputs.target_repo_name }}-git-${{ runner.os }}- - name: Check out target repository + id: checkout-target if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' }} + env: + ITEM_KIND: ${{ steps.live-item.outputs.item_kind }} + ITEM_NUMBER: ${{ steps.target.outputs.item_number }} + SOURCE_HEAD_SHA: ${{ fromJSON(steps.claim-exact-review-queue.outputs.decision).sourceHeadSha || '' }} run: | set -euo pipefail url="https://github.com/${{ steps.target.outputs.target_repo }}.git" @@ -887,10 +892,39 @@ jobs: git clone --bare --filter=blob:none --single-branch --branch "$target_branch" "$url" "$cache_dir" git clone --filter=blob:none --branch "$target_branch" --single-branch "$url" "$checkout_dir" fi + if [ "$ITEM_KIND" = "pull_request" ]; then + if ! [[ "$SOURCE_HEAD_SHA" =~ ^[0-9a-f]{40}$ ]]; then + echo "::error::Exact PR review requires a valid leased source head SHA." + exit 1 + fi + git -C "$checkout_dir" fetch --force --depth=50 origin "refs/pull/${ITEM_NUMBER}/head" + fetched_head="$(git -C "$checkout_dir" rev-parse FETCH_HEAD)" + if [ "$fetched_head" != "$SOURCE_HEAD_SHA" ]; then + echo "::notice::Fetched PR head $fetched_head moved past leased source head $SOURCE_HEAD_SHA; skipping review and requeueing the latest source." + echo "source_drift=true" >> "$GITHUB_OUTPUT" + exit 0 + else + git -C "$checkout_dir" checkout --detach "$SOURCE_HEAD_SHA" + checked_out_head="$(git -C "$checkout_dir" rev-parse HEAD)" + if [ "$checked_out_head" != "$SOURCE_HEAD_SHA" ]; then + echo "::error::Target checkout head $checked_out_head does not match leased source head $SOURCE_HEAD_SHA." + exit 1 + fi + fi + fi + echo "source_drift=false" >> "$GITHUB_OUTPUT" git -C "$checkout_dir" rev-parse --short HEAD + - name: Requeue exact review after source drift + if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.checkout-target.outputs.source_drift == 'true' }} + run: echo "::notice::The leased PR source moved before review; this run is a successful no-op and the durable queue will review the latest head." + + - name: Materialize Codex source for OpenClaw review + if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.checkout-target.outputs.source_drift != 'true' && steps.target.outputs.target_repo == 'openclaw/openclaw' }} + run: git clone --filter=blob:none --depth=1 https://github.com/openai/codex.git codex + - name: Mark re-review command in progress - if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' }} + if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.checkout-target.outputs.source_drift != 'true' }} continue-on-error: true env: GH_TOKEN: ${{ steps.target-write-token.outputs.token }} @@ -911,7 +945,7 @@ jobs: --wait-ms 120000 - uses: ./.github/actions/setup-codex - if: ${{ env.CLAWSWEEPER_RUNNER != 'openclaw' && steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' }} + if: ${{ env.CLAWSWEEPER_RUNNER != 'openclaw' && steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.checkout-target.outputs.source_drift != 'true' }} env: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} CLAWSWEEPER_INTERNAL_MODEL: ${{ secrets.CLAWSWEEPER_MODEL }} @@ -919,11 +953,11 @@ jobs: login-status: "true" - uses: ./.github/actions/setup-openclaw - if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' }} + if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.checkout-target.outputs.source_drift != 'true' }} - name: Reserve exact review lease id: reserve-exact-review-lease - if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' }} + if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.checkout-target.outputs.source_drift != 'true' }} env: GH_TOKEN: ${{ steps.target-write-token.outputs.token }} TARGET_REPO: ${{ steps.target.outputs.target_repo }} @@ -1232,7 +1266,7 @@ jobs: fi - name: Finalize exact event action ledger - if: ${{ always() && steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' }} + if: ${{ always() && steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.checkout-target.outputs.source_drift != 'true' }} continue-on-error: true env: REVIEW_EXIT_CODE: ${{ steps.review-exact-event-item.outputs.exit_code || '' }} @@ -1251,7 +1285,7 @@ jobs: - name: Create exact review artifact bundle id: create-exact-review-bundle - if: ${{ always() && steps.claim-exact-review-queue.outputs.claimed == 'true' && !cancelled() && steps.target.outputs.target_enabled == 'true' && steps.live-item.outcome == 'success' && steps.live-item.outputs.admission_retry != 'true' && steps.setup-pnpm.outcome == 'success' && steps.review-exact-event-item.outputs.superseded != 'true' && (steps.live-item.outputs.proceed != 'true' || (steps.review-exact-event-item.outcome == 'success' && steps.review-exact-event-item.outputs.retry_at == '')) }} + if: ${{ always() && steps.claim-exact-review-queue.outputs.claimed == 'true' && !cancelled() && steps.target.outputs.target_enabled == 'true' && steps.live-item.outcome == 'success' && steps.live-item.outputs.admission_retry != 'true' && steps.setup-pnpm.outcome == 'success' && steps.checkout-target.outputs.source_drift != 'true' && steps.review-exact-event-item.outputs.superseded != 'true' && (steps.live-item.outputs.proceed != 'true' || (steps.review-exact-event-item.outcome == 'success' && steps.review-exact-event-item.outputs.retry_at == '')) }} env: EXACT_REVIEW_ACTION_LEDGER_ROOT: ${{ env.CLAWSWEEPER_ACTION_LEDGER_OUTPUT_ROOT }} EXACT_REVIEW_BUNDLE_DIR: .artifacts/exact-review-bundle @@ -1471,13 +1505,17 @@ jobs: RUN_URL: https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }} REVIEW_OUTCOME: ${{ steps.review-exact-event-item.outcome }} REVIEW_SUPERSEDED: ${{ steps.review-exact-event-item.outputs.superseded || 'false' }} + SOURCE_DRIFT: ${{ steps.checkout-target.outputs.source_drift || 'false' }} RESERVATION_STATUS: ${{ steps.reserve-exact-review-lease.outputs.status }} RETRY_AT: ${{ steps.reserve-exact-review-lease.outputs.retry_at || steps.review-exact-event-item.outputs.retry_at }} CLAWSWEEPER_ACTION_LEDGER_DISABLED: "1" run: | state="Failed" detail="The exact review did not produce a publishable artifact. The durable queue will retry it." - if [ "$RESERVATION_STATUS" = "superseded" ] || [ "$REVIEW_SUPERSEDED" = "true" ]; then + if [ "$SOURCE_DRIFT" = "true" ]; then + state="Waiting" + detail="The pull request head moved before review. The durable queue will review the latest source." + elif [ "$RESERVATION_STATUS" = "superseded" ] || [ "$REVIEW_SUPERSEDED" = "true" ]; then state="Waiting" detail="A newer exact-review queue owner superseded this run. This run completed as a no-op." elif [ "$RESERVATION_STATUS" = "held" ]; then @@ -1505,13 +1543,17 @@ jobs: LIVE_OUTCOME: ${{ steps.live-item.outcome }} REVIEW_OUTCOME: ${{ steps.review-exact-event-item.outcome }} REVIEW_SUPERSEDED: ${{ steps.review-exact-event-item.outputs.superseded || 'false' }} + SOURCE_DRIFT: ${{ steps.checkout-target.outputs.source_drift || 'false' }} RESERVATION_STATUS: ${{ steps.reserve-exact-review-lease.outputs.status }} PUBLICATION_QUEUE_OUTCOME: ${{ steps.queue-exact-review-publication.outcome }} DIRECT_PUBLICATION_ACCEPTED: ${{ steps.direct-exact-review-publication.outputs.accepted }} run: | outcome=failure requeue_latest=false - if [ "$ADMISSION_RETRY" = "true" ]; then + if [ "$SOURCE_DRIFT" = "true" ]; then + outcome=success + requeue_latest=true + elif [ "$ADMISSION_RETRY" = "true" ]; then outcome=success requeue_latest=true elif [ "$TARGET_ENABLED" = "false" ]; then @@ -3290,6 +3332,10 @@ jobs: fi git -C "$checkout_dir" rev-parse --short HEAD + - name: Materialize Codex source for OpenClaw review + if: ${{ needs.plan.outputs.target_repo == 'openclaw/openclaw' }} + run: git clone --filter=blob:none --depth=1 https://github.com/openai/codex.git codex + - name: Mark shard start id: shard-start run: echo "started_at=$(date -u '+%Y-%m-%dT%H:%M:%SZ')" >> "$GITHUB_OUTPUT" diff --git a/test/sweep-workflow.test.ts b/test/sweep-workflow.test.ts index 208b1a20e8..0d7658ae93 100644 --- a/test/sweep-workflow.test.ts +++ b/test/sweep-workflow.test.ts @@ -3610,6 +3610,78 @@ test("sweep target checkouts retry without cached references", () => { } }); +test("exact PR reviews fail closed unless the leased source head is checked out", () => { + const workflow = YAML.parse(readText(".github/workflows/sweep.yml")) as { + jobs: Record< + string, + { + steps: Array<{ + name?: string; + if?: string; + env?: Record; + run?: string; + }>; + } + >; + }; + const steps = workflow.jobs["event-review-apply"]!.steps; + const checkoutIndex = steps.findIndex((step) => step.name === "Check out target repository"); + const codexIndex = steps.findIndex( + (step) => step.name === "Materialize Codex source for OpenClaw review", + ); + const reviewIndex = steps.findIndex((step) => step.name === "Review exact event item"); + const checkout = steps[checkoutIndex]!; + const codex = steps[codexIndex]!; + const requeue = steps.find((step) => step.name === "Requeue exact review after source drift")!; + const generation = steps.find((step) => step.name === "Export exact review generation result")!; + + assert.ok(checkoutIndex >= 0); + assert.ok(codexIndex > checkoutIndex); + assert.ok(reviewIndex > codexIndex); + assert.equal( + checkout.env?.SOURCE_HEAD_SHA, + "${{ fromJSON(steps.claim-exact-review-queue.outputs.decision).sourceHeadSha || '' }}", + ); + assert.match(checkout.run ?? "", /refs\/pull\/\$\{ITEM_NUMBER\}\/head/); + assert.match(checkout.run ?? "", /fetched_head" != "\$SOURCE_HEAD_SHA/); + assert.match(checkout.run ?? "", /source_drift=true/); + assert.match(checkout.run ?? "", /exit 0/); + assert.match(checkout.run ?? "", /checkout --detach "\$SOURCE_HEAD_SHA"/); + assert.match(checkout.run ?? "", /checked_out_head" != "\$SOURCE_HEAD_SHA/); + assert.equal( + requeue.if, + "${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.checkout-target.outputs.source_drift == 'true' }}", + ); + assert.match(codex.if ?? "", /source_drift != 'true'/); + assert.match(codex.if ?? "", /target_repo == 'openclaw\/openclaw'/); + assert.match(codex.run ?? "", /https:\/\/github\.com\/openai\/codex\.git codex/); + assert.equal( + generation.env?.SOURCE_DRIFT, + "${{ steps.checkout-target.outputs.source_drift || 'false' }}", + ); + assert.match(generation.run ?? "", /SOURCE_DRIFT" = "true"/); + assert.match(generation.run ?? "", /requeue_latest=true/); +}); + +test("scheduled OpenClaw review workers materialize sibling Codex source", () => { + const workflow = YAML.parse(readText(".github/workflows/sweep.yml")) as { + jobs: Record }>; + }; + const steps = workflow.jobs.review!.steps; + const checkoutIndex = steps.findIndex((step) => step.name === "Check out target repository"); + const codexIndex = steps.findIndex( + (step) => step.name === "Materialize Codex source for OpenClaw review", + ); + const reviewIndex = steps.findIndex((step) => step.name === "Review shard"); + const codex = steps[codexIndex]!; + + assert.ok(checkoutIndex >= 0); + assert.ok(codexIndex > checkoutIndex); + assert.ok(reviewIndex > codexIndex); + assert.equal(codex.if, "${{ needs.plan.outputs.target_repo == 'openclaw/openclaw' }}"); + assert.match(codex.run ?? "", /https:\/\/github\.com\/openai\/codex\.git codex/); +}); + test("target sweep runs count as background review capacity", () => { const workflow = readText(".github/workflows/sweep.yml"); const capacityBlock = workflow.slice( From 9da9049ed697594aff34a1391972179b5500358d Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 31 Jul 2026 22:37:01 +0200 Subject: [PATCH 2/3] refactor(review): test leased source materialization --- .github/workflows/sweep.yml | 31 ++----- CHANGELOG.md | 1 + package.json | 1 + src/repair/exact-review-source-cli.ts | 71 +++++++++++++++ src/repair/exact-review-source.ts | 61 +++++++++++++ test/repair/exact-review-source.test.ts | 115 ++++++++++++++++++++++++ test/sweep-workflow.test.ts | 29 ++++-- 7 files changed, 276 insertions(+), 33 deletions(-) create mode 100644 src/repair/exact-review-source-cli.ts create mode 100644 src/repair/exact-review-source.ts create mode 100644 test/repair/exact-review-source.test.ts diff --git a/.github/workflows/sweep.yml b/.github/workflows/sweep.yml index 334166ce52..5af7f25a6b 100644 --- a/.github/workflows/sweep.yml +++ b/.github/workflows/sweep.yml @@ -892,28 +892,11 @@ jobs: git clone --bare --filter=blob:none --single-branch --branch "$target_branch" "$url" "$cache_dir" git clone --filter=blob:none --branch "$target_branch" --single-branch "$url" "$checkout_dir" fi - if [ "$ITEM_KIND" = "pull_request" ]; then - if ! [[ "$SOURCE_HEAD_SHA" =~ ^[0-9a-f]{40}$ ]]; then - echo "::error::Exact PR review requires a valid leased source head SHA." - exit 1 - fi - git -C "$checkout_dir" fetch --force --depth=50 origin "refs/pull/${ITEM_NUMBER}/head" - fetched_head="$(git -C "$checkout_dir" rev-parse FETCH_HEAD)" - if [ "$fetched_head" != "$SOURCE_HEAD_SHA" ]; then - echo "::notice::Fetched PR head $fetched_head moved past leased source head $SOURCE_HEAD_SHA; skipping review and requeueing the latest source." - echo "source_drift=true" >> "$GITHUB_OUTPUT" - exit 0 - else - git -C "$checkout_dir" checkout --detach "$SOURCE_HEAD_SHA" - checked_out_head="$(git -C "$checkout_dir" rev-parse HEAD)" - if [ "$checked_out_head" != "$SOURCE_HEAD_SHA" ]; then - echo "::error::Target checkout head $checked_out_head does not match leased source head $SOURCE_HEAD_SHA." - exit 1 - fi - fi - fi - echo "source_drift=false" >> "$GITHUB_OUTPUT" - git -C "$checkout_dir" rev-parse --short HEAD + pnpm run --silent repair:exact-review-source -- \ + --target-dir "$checkout_dir" \ + --item-kind "$ITEM_KIND" \ + --item-number "$ITEM_NUMBER" \ + --source-head-sha "$SOURCE_HEAD_SHA" - name: Requeue exact review after source drift if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.checkout-target.outputs.source_drift == 'true' }} @@ -1062,7 +1045,7 @@ jobs: - name: Review exact event item id: review-exact-event-item - if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.reserve-exact-review-lease.outputs.status == 'posted' }} + if: ${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.checkout-target.outputs.source_drift != 'true' && steps.reserve-exact-review-lease.outputs.status == 'posted' }} continue-on-error: true env: GH_TOKEN: ${{ steps.target-read-token.outputs.token }} @@ -1465,7 +1448,7 @@ jobs: exit 1 - name: Release unsuccessful workflow-owned review lease - if: ${{ always() && steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.reserve-exact-review-lease.outputs.status != 'held' && steps.direct-exact-review-publication.outputs.accepted != 'true' && steps.queue-exact-review-publication.outcome != 'success' }} + if: ${{ always() && steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.live-item.outputs.proceed == 'true' && steps.checkout-target.outputs.source_drift != 'true' && steps.reserve-exact-review-lease.outputs.status != 'held' && steps.direct-exact-review-publication.outputs.accepted != 'true' && steps.queue-exact-review-publication.outcome != 'success' }} env: GH_TOKEN: ${{ steps.target-write-token.outputs.token }} TARGET_REPO: ${{ steps.target.outputs.target_repo }} diff --git a/CHANGELOG.md b/CHANGELOG.md index edd353ad1b..28a58730d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ checkpoint, and status-only commits are intentionally omitted. ### Changed +- Exact pull-request reviews now materialize and verify the leased head through a tested TypeScript guard, requeue moved heads without running review or lease cleanup, and materialize sibling Codex source for OpenClaw contract review. - Event-review artifact publication completes as a superseded no-op when the reviewed branch vanished upstream (force-push or deletion) instead of failing the run. - Worker record requests now retry transient blank/invalid 2xx bodies from the edge within the bounded budget instead of failing hydration on the first occurrence. - Exact reviews of items that closed after enqueue now complete as superseded no-ops, and GitHub-throttled reservations defer as held retries — neither spends the item's review-failure budget. diff --git a/package.json b/package.json index 9b5343920b..712199736a 100644 --- a/package.json +++ b/package.json @@ -62,6 +62,7 @@ "repair:spam-comment-intake": "node dist/repair/spam-comment-intake.js", "repair:spam-scan": "node dist/repair/spam-scanner.js", "repair:exact-review-bundle": "node dist/repair/exact-review-bundle-cli.js", + "repair:exact-review-source": "node dist/repair/exact-review-source-cli.js", "repair:exact-review-queue-maintenance": "node dist/repair/exact-review-queue-maintenance.js", "repair:scheduled-review-enqueue": "node dist/repair/scheduled-review-enqueue.js", "repair:exact-review-dead-letters": "node scripts/exact-review-dead-letter-operator.mjs", diff --git a/src/repair/exact-review-source-cli.ts b/src/repair/exact-review-source-cli.ts new file mode 100644 index 0000000000..4d6c936472 --- /dev/null +++ b/src/repair/exact-review-source-cli.ts @@ -0,0 +1,71 @@ +#!/usr/bin/env node +import fs from "node:fs"; + +import { materializeExactReviewSource } from "./exact-review-source.js"; + +const args = parseArgs(process.argv.slice(2)); +const result = materializeExactReviewSource({ + targetDir: required(args.targetDir, "--target-dir"), + itemKind: itemKind(required(args.itemKind, "--item-kind")), + itemNumber: positiveInteger(required(args.itemNumber, "--item-number"), "--item-number"), + ...(args.sourceHeadSha ? { sourceHeadSha: args.sourceHeadSha } : {}), +}); + +writeOutput("status", result.status); +writeOutput("source_drift", result.status === "source_drift" ? "true" : "false"); +if (result.status === "source_drift") { + writeOutput("fetched_head_sha", result.fetchedHeadSha); + console.error( + `Fetched PR head ${result.fetchedHeadSha} moved past leased source head ${result.leasedHeadSha}; skipping review so the durable queue can requeue the latest source.`, + ); +} else { + writeOutput("materialized_head_sha", result.headSha); + console.log(result.headSha.slice(0, 12)); +} + +function parseArgs(argv: readonly string[]) { + const parsed: { + targetDir?: string; + itemKind?: string; + itemNumber?: string; + sourceHeadSha?: string; + } = {}; + for (let index = 0; index < argv.length; index += 1) { + const flag = argv[index]; + const value = argv[index + 1]; + if (value === undefined || (flag !== "--source-head-sha" && value.startsWith("--"))) { + throw new Error(`${flag} requires a value`); + } + if (flag === "--target-dir") parsed.targetDir = value; + else if (flag === "--item-kind") parsed.itemKind = value; + else if (flag === "--item-number") parsed.itemNumber = value; + else if (flag === "--source-head-sha") parsed.sourceHeadSha = value; + else throw new Error(`unknown argument: ${flag}`); + index += 1; + } + return parsed; +} + +function required(value: string | undefined, flag: string): string { + if (!value) throw new Error(`${flag} is required`); + return value; +} + +function itemKind(value: string): "issue" | "pull_request" { + if (value === "issue" || value === "pull_request") return value; + throw new Error("--item-kind must be issue or pull_request"); +} + +function positiveInteger(value: string, flag: string): number { + const parsed = Number(value); + if (!Number.isSafeInteger(parsed) || parsed < 1) { + throw new Error(`${flag} must be a positive integer`); + } + return parsed; +} + +function writeOutput(name: string, value: string): void { + const outputPath = process.env.GITHUB_OUTPUT; + if (outputPath) fs.appendFileSync(outputPath, `${name}=${value}\n`, "utf8"); + else console.log(`${name}=${value}`); +} diff --git a/src/repair/exact-review-source.ts b/src/repair/exact-review-source.ts new file mode 100644 index 0000000000..390441c7e6 --- /dev/null +++ b/src/repair/exact-review-source.ts @@ -0,0 +1,61 @@ +import { currentHead } from "./git-repo-utils.js"; +import { runCommand as run } from "./command-runner.js"; + +const gitNetworkTimeoutMs = Math.max( + 30_000, + Number( + process.env.CLAWSWEEPER_GIT_NETWORK_TIMEOUT_MS ?? + process.env.CLAWSWEEPER_NETWORK_COMMAND_TIMEOUT_MS ?? + 5 * 60 * 1000, + ), +); + +export type ExactReviewSourceResult = + | { status: "ready"; headSha: string } + | { status: "source_drift"; leasedHeadSha: string; fetchedHeadSha: string }; + +export function materializeExactReviewSource(options: { + targetDir: string; + itemKind: "issue" | "pull_request"; + itemNumber: number; + sourceHeadSha?: string; +}): ExactReviewSourceResult { + if (!Number.isSafeInteger(options.itemNumber) || options.itemNumber < 1) { + throw new Error("itemNumber must be a positive integer"); + } + if (options.itemKind === "issue") { + return { status: "ready", headSha: currentHead(options.targetDir) }; + } + + const leasedHeadSha = String(options.sourceHeadSha ?? "") + .trim() + .toLowerCase(); + if (!/^[0-9a-f]{40}$/.test(leasedHeadSha)) { + throw new Error("Exact PR review requires a valid leased source head SHA"); + } + + run("git", ["fetch", "--force", "--depth=50", "origin", `refs/pull/${options.itemNumber}/head`], { + cwd: options.targetDir, + timeoutMs: gitNetworkTimeoutMs, + }); + const fetchedHeadSha = run("git", ["rev-parse", "FETCH_HEAD"], { + cwd: options.targetDir, + }) + .trim() + .toLowerCase(); + if (!/^[0-9a-f]{40}$/.test(fetchedHeadSha)) { + throw new Error(`Fetched PR head is not a full commit SHA: ${fetchedHeadSha || "empty"}`); + } + if (fetchedHeadSha !== leasedHeadSha) { + return { status: "source_drift", leasedHeadSha, fetchedHeadSha }; + } + + run("git", ["checkout", "--detach", leasedHeadSha], { cwd: options.targetDir }); + const headSha = currentHead(options.targetDir).toLowerCase(); + if (headSha !== leasedHeadSha) { + throw new Error( + `Target checkout head ${headSha || "empty"} does not match leased source head ${leasedHeadSha}`, + ); + } + return { status: "ready", headSha }; +} diff --git a/test/repair/exact-review-source.test.ts b/test/repair/exact-review-source.test.ts new file mode 100644 index 0000000000..068154646d --- /dev/null +++ b/test/repair/exact-review-source.test.ts @@ -0,0 +1,115 @@ +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; + +import { materializeExactReviewSource } from "../../dist/repair/exact-review-source.js"; + +function git(args: string[], cwd?: string): string { + return execFileSync("git", args, { + ...(cwd ? { cwd } : {}), + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }).trim(); +} + +function createPullFixture() { + const root = mkdtempSync(join(tmpdir(), "exact-review-source-")); + const origin = join(root, "origin.git"); + const source = join(root, "source"); + const target = join(root, "target"); + + git(["init", "--bare", origin]); + git(["init", source]); + git(["config", "user.email", "clawsweeper@example.com"], source); + git(["config", "user.name", "ClawSweeper Test"], source); + writeFileSync(join(source, "README.md"), "base\n"); + git(["add", "README.md"], source); + git(["commit", "-m", "base"], source); + git(["branch", "-M", "main"], source); + git(["remote", "add", "origin", origin], source); + git(["push", "origin", "main"], source); + + writeFileSync(join(source, "feature.txt"), "first\n"); + git(["add", "feature.txt"], source); + git(["commit", "-m", "feature"], source); + const leasedHeadSha = git(["rev-parse", "HEAD"], source); + git(["push", "origin", "HEAD:refs/pull/357/head"], source); + git(["clone", "--branch", "main", origin, target]); + + return { root, origin, source, target, leasedHeadSha }; +} + +test("materializes the immutable leased pull request head", () => { + const fixture = createPullFixture(); + try { + assert.deepEqual( + materializeExactReviewSource({ + targetDir: fixture.target, + itemKind: "pull_request", + itemNumber: 357, + sourceHeadSha: fixture.leasedHeadSha, + }), + { status: "ready", headSha: fixture.leasedHeadSha }, + ); + assert.equal(git(["rev-parse", "HEAD"], fixture.target), fixture.leasedHeadSha); + assert.equal(git(["rev-parse", "--abbrev-ref", "HEAD"], fixture.target), "HEAD"); + assert.equal(readFileSync(join(fixture.target, "feature.txt"), "utf8"), "first\n"); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } +}); + +test("reports source drift without checking out the moved pull request head", () => { + const fixture = createPullFixture(); + try { + const originalCheckoutHead = git(["rev-parse", "HEAD"], fixture.target); + writeFileSync(join(fixture.source, "feature.txt"), "second\n"); + git(["add", "feature.txt"], fixture.source); + git(["commit", "-m", "move head"], fixture.source); + const fetchedHeadSha = git(["rev-parse", "HEAD"], fixture.source); + git(["push", "--force", "origin", "HEAD:refs/pull/357/head"], fixture.source); + + assert.deepEqual( + materializeExactReviewSource({ + targetDir: fixture.target, + itemKind: "pull_request", + itemNumber: 357, + sourceHeadSha: fixture.leasedHeadSha, + }), + { + status: "source_drift", + leasedHeadSha: fixture.leasedHeadSha, + fetchedHeadSha, + }, + ); + assert.equal(git(["rev-parse", "HEAD"], fixture.target), originalCheckoutHead); + assert.equal(existsSync(join(fixture.target, "feature.txt")), false); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } +}); + +test("fails closed when the pull request ref cannot be fetched", () => { + const root = mkdtempSync(join(tmpdir(), "exact-review-source-missing-")); + const origin = join(root, "origin.git"); + const target = join(root, "target"); + try { + git(["init", "--bare", origin]); + git(["clone", origin, target]); + assert.throws( + () => + materializeExactReviewSource({ + targetDir: target, + itemKind: "pull_request", + itemNumber: 357, + sourceHeadSha: "a".repeat(40), + }), + /couldn't find remote ref|could not read from remote repository|fatal:/i, + ); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); diff --git a/test/sweep-workflow.test.ts b/test/sweep-workflow.test.ts index 0d7658ae93..8a7dfaf10e 100644 --- a/test/sweep-workflow.test.ts +++ b/test/sweep-workflow.test.ts @@ -3594,9 +3594,13 @@ test("sweep workflow schedules cursor-based PR comment sync batches", () => { }); test("sweep target checkouts retry without cached references", () => { - const workflow = readText(".github/workflows/sweep.yml"); - const checkoutBlocks = - workflow.match(/- name: Check out target repository[\s\S]*?rev-parse --short HEAD/g) ?? []; + const workflow = YAML.parse(readText(".github/workflows/sweep.yml")) as { + jobs: Record }>; + }; + const checkoutBlocks = Object.values(workflow.jobs) + .flatMap((job) => job.steps ?? []) + .filter((step) => step.name === "Check out target repository") + .map((step) => step.run ?? ""); assert.equal(checkoutBlocks.length, 2); for (const block of checkoutBlocks) { @@ -3642,12 +3646,7 @@ test("exact PR reviews fail closed unless the leased source head is checked out" checkout.env?.SOURCE_HEAD_SHA, "${{ fromJSON(steps.claim-exact-review-queue.outputs.decision).sourceHeadSha || '' }}", ); - assert.match(checkout.run ?? "", /refs\/pull\/\$\{ITEM_NUMBER\}\/head/); - assert.match(checkout.run ?? "", /fetched_head" != "\$SOURCE_HEAD_SHA/); - assert.match(checkout.run ?? "", /source_drift=true/); - assert.match(checkout.run ?? "", /exit 0/); - assert.match(checkout.run ?? "", /checkout --detach "\$SOURCE_HEAD_SHA"/); - assert.match(checkout.run ?? "", /checked_out_head" != "\$SOURCE_HEAD_SHA/); + assert.match(checkout.run ?? "", /repair:exact-review-source/); assert.equal( requeue.if, "${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.checkout-target.outputs.source_drift == 'true' }}", @@ -3661,6 +3660,18 @@ test("exact PR reviews fail closed unless the leased source head is checked out" ); assert.match(generation.run ?? "", /SOURCE_DRIFT" = "true"/); assert.match(generation.run ?? "", /requeue_latest=true/); + + for (const name of [ + "Reserve exact review lease", + "Review exact event item", + "Finalize exact event action ledger", + "Create exact review artifact bundle", + "Release unsuccessful workflow-owned review lease", + ]) { + const step = steps.find((candidate) => candidate.name === name); + assert.ok(step, `missing ${name}`); + assert.match(step.if ?? "", /checkout-target\.outputs\.source_drift != 'true'/); + } }); test("scheduled OpenClaw review workers materialize sibling Codex source", () => { From 813167fd14590cbfa5ae5a65dcdace44e815647a Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 31 Jul 2026 22:48:47 +0200 Subject: [PATCH 3/3] fix(review): preserve exact review ancestry --- .github/workflows/sweep.yml | 1 + CHANGELOG.md | 2 +- src/repair/exact-review-source-cli.ts | 6 ++- src/repair/exact-review-source.ts | 10 ++++- test/repair/exact-review-source.test.ts | 59 ++++++++++++++++++++++--- test/sweep-workflow.test.ts | 1 + 6 files changed, 68 insertions(+), 11 deletions(-) diff --git a/.github/workflows/sweep.yml b/.github/workflows/sweep.yml index 5af7f25a6b..c238dcd072 100644 --- a/.github/workflows/sweep.yml +++ b/.github/workflows/sweep.yml @@ -896,6 +896,7 @@ jobs: --target-dir "$checkout_dir" \ --item-kind "$ITEM_KIND" \ --item-number "$ITEM_NUMBER" \ + --base-branch "$target_branch" \ --source-head-sha "$SOURCE_HEAD_SHA" - name: Requeue exact review after source drift diff --git a/CHANGELOG.md b/CHANGELOG.md index 28a58730d3..7b66e038d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ checkpoint, and status-only commits are intentionally omitted. ### Changed -- Exact pull-request reviews now materialize and verify the leased head through a tested TypeScript guard, requeue moved heads without running review or lease cleanup, and materialize sibling Codex source for OpenClaw contract review. +- Exact pull-request reviews now materialize and verify the leased head with complete merge-base ancestry through a tested TypeScript guard, requeue moved heads without running review or lease cleanup, and materialize sibling Codex source for OpenClaw contract review. - Event-review artifact publication completes as a superseded no-op when the reviewed branch vanished upstream (force-push or deletion) instead of failing the run. - Worker record requests now retry transient blank/invalid 2xx bodies from the edge within the bounded budget instead of failing hydration on the first occurrence. - Exact reviews of items that closed after enqueue now complete as superseded no-ops, and GitHub-throttled reservations defer as held retries — neither spends the item's review-failure budget. diff --git a/src/repair/exact-review-source-cli.ts b/src/repair/exact-review-source-cli.ts index 4d6c936472..fb83fc52f5 100644 --- a/src/repair/exact-review-source-cli.ts +++ b/src/repair/exact-review-source-cli.ts @@ -4,10 +4,12 @@ import fs from "node:fs"; import { materializeExactReviewSource } from "./exact-review-source.js"; const args = parseArgs(process.argv.slice(2)); +const kind = itemKind(required(args.itemKind, "--item-kind")); const result = materializeExactReviewSource({ targetDir: required(args.targetDir, "--target-dir"), - itemKind: itemKind(required(args.itemKind, "--item-kind")), + itemKind: kind, itemNumber: positiveInteger(required(args.itemNumber, "--item-number"), "--item-number"), + ...(kind === "pull_request" ? { baseBranch: required(args.baseBranch, "--base-branch") } : {}), ...(args.sourceHeadSha ? { sourceHeadSha: args.sourceHeadSha } : {}), }); @@ -28,6 +30,7 @@ function parseArgs(argv: readonly string[]) { targetDir?: string; itemKind?: string; itemNumber?: string; + baseBranch?: string; sourceHeadSha?: string; } = {}; for (let index = 0; index < argv.length; index += 1) { @@ -39,6 +42,7 @@ function parseArgs(argv: readonly string[]) { if (flag === "--target-dir") parsed.targetDir = value; else if (flag === "--item-kind") parsed.itemKind = value; else if (flag === "--item-number") parsed.itemNumber = value; + else if (flag === "--base-branch") parsed.baseBranch = value; else if (flag === "--source-head-sha") parsed.sourceHeadSha = value; else throw new Error(`unknown argument: ${flag}`); index += 1; diff --git a/src/repair/exact-review-source.ts b/src/repair/exact-review-source.ts index 390441c7e6..cc134ad4b9 100644 --- a/src/repair/exact-review-source.ts +++ b/src/repair/exact-review-source.ts @@ -1,4 +1,4 @@ -import { currentHead } from "./git-repo-utils.js"; +import { currentHead, ensureMergeBaseAvailable } from "./git-repo-utils.js"; import { runCommand as run } from "./command-runner.js"; const gitNetworkTimeoutMs = Math.max( @@ -18,6 +18,7 @@ export function materializeExactReviewSource(options: { targetDir: string; itemKind: "issue" | "pull_request"; itemNumber: number; + baseBranch?: string; sourceHeadSha?: string; }): ExactReviewSourceResult { if (!Number.isSafeInteger(options.itemNumber) || options.itemNumber < 1) { @@ -33,8 +34,12 @@ export function materializeExactReviewSource(options: { if (!/^[0-9a-f]{40}$/.test(leasedHeadSha)) { throw new Error("Exact PR review requires a valid leased source head SHA"); } + const baseBranch = String(options.baseBranch ?? "").trim(); + if (!baseBranch) { + throw new Error("Exact PR review requires its base branch"); + } - run("git", ["fetch", "--force", "--depth=50", "origin", `refs/pull/${options.itemNumber}/head`], { + run("git", ["fetch", "--force", "origin", `refs/pull/${options.itemNumber}/head`], { cwd: options.targetDir, timeoutMs: gitNetworkTimeoutMs, }); @@ -57,5 +62,6 @@ export function materializeExactReviewSource(options: { `Target checkout head ${headSha || "empty"} does not match leased source head ${leasedHeadSha}`, ); } + ensureMergeBaseAvailable({ targetDir: options.targetDir, baseBranch }); return { status: "ready", headSha }; } diff --git a/test/repair/exact-review-source.test.ts b/test/repair/exact-review-source.test.ts index 068154646d..0a38b1e6b9 100644 --- a/test/repair/exact-review-source.test.ts +++ b/test/repair/exact-review-source.test.ts @@ -15,7 +15,10 @@ function git(args: string[], cwd?: string): string { }).trim(); } -function createPullFixture() { +function createPullFixture( + featureCommitCount = 1, + options: { baseAdvanceCount?: number; shallowTarget?: boolean } = {}, +) { const root = mkdtempSync(join(tmpdir(), "exact-review-source-")); const origin = join(root, "origin.git"); const source = join(root, "source"); @@ -28,18 +31,36 @@ function createPullFixture() { writeFileSync(join(source, "README.md"), "base\n"); git(["add", "README.md"], source); git(["commit", "-m", "base"], source); + const baseSha = git(["rev-parse", "HEAD"], source); git(["branch", "-M", "main"], source); git(["remote", "add", "origin", origin], source); git(["push", "origin", "main"], source); - writeFileSync(join(source, "feature.txt"), "first\n"); - git(["add", "feature.txt"], source); - git(["commit", "-m", "feature"], source); + for (let index = 1; index <= featureCommitCount; index += 1) { + writeFileSync(join(source, "feature.txt"), `feature ${index}\n`); + git(["add", "feature.txt"], source); + git(["commit", "-m", `feature ${index}`], source); + } const leasedHeadSha = git(["rev-parse", "HEAD"], source); git(["push", "origin", "HEAD:refs/pull/357/head"], source); - git(["clone", "--branch", "main", origin, target]); + if ((options.baseAdvanceCount ?? 0) > 0) { + git(["checkout", "-B", "advanced-main", baseSha], source); + for (let index = 1; index <= (options.baseAdvanceCount ?? 0); index += 1) { + writeFileSync(join(source, "README.md"), `base ${index}\n`); + git(["add", "README.md"], source); + git(["commit", "-m", `base ${index}`], source); + } + git(["push", "origin", "HEAD:main"], source); + } + git([ + "clone", + "--branch", + "main", + ...(options.shallowTarget ? ["--depth=1", `file://${origin}`] : [origin]), + target, + ]); - return { root, origin, source, target, leasedHeadSha }; + return { root, origin, source, target, baseSha, leasedHeadSha }; } test("materializes the immutable leased pull request head", () => { @@ -50,13 +71,35 @@ test("materializes the immutable leased pull request head", () => { targetDir: fixture.target, itemKind: "pull_request", itemNumber: 357, + baseBranch: "main", sourceHeadSha: fixture.leasedHeadSha, }), { status: "ready", headSha: fixture.leasedHeadSha }, ); assert.equal(git(["rev-parse", "HEAD"], fixture.target), fixture.leasedHeadSha); assert.equal(git(["rev-parse", "--abbrev-ref", "HEAD"], fixture.target), "HEAD"); - assert.equal(readFileSync(join(fixture.target, "feature.txt"), "utf8"), "first\n"); + assert.equal(readFileSync(join(fixture.target, "feature.txt"), "utf8"), "feature 1\n"); + } finally { + rmSync(fixture.root, { recursive: true, force: true }); + } +}); + +test("materializes both sides of deep pull request ancestry from a shallow base", () => { + const fixture = createPullFixture(75, { baseAdvanceCount: 60, shallowTarget: true }); + try { + assert.deepEqual( + materializeExactReviewSource({ + targetDir: fixture.target, + itemKind: "pull_request", + itemNumber: 357, + baseBranch: "main", + sourceHeadSha: fixture.leasedHeadSha, + }), + { status: "ready", headSha: fixture.leasedHeadSha }, + ); + assert.equal(git(["merge-base", "origin/main", "HEAD"], fixture.target), fixture.baseSha); + assert.equal(git(["rev-list", "--count", "origin/main..HEAD"], fixture.target), "75"); + assert.equal(git(["rev-parse", "--is-shallow-repository"], fixture.target), "false"); } finally { rmSync(fixture.root, { recursive: true, force: true }); } @@ -77,6 +120,7 @@ test("reports source drift without checking out the moved pull request head", () targetDir: fixture.target, itemKind: "pull_request", itemNumber: 357, + baseBranch: "main", sourceHeadSha: fixture.leasedHeadSha, }), { @@ -105,6 +149,7 @@ test("fails closed when the pull request ref cannot be fetched", () => { targetDir: target, itemKind: "pull_request", itemNumber: 357, + baseBranch: "main", sourceHeadSha: "a".repeat(40), }), /couldn't find remote ref|could not read from remote repository|fatal:/i, diff --git a/test/sweep-workflow.test.ts b/test/sweep-workflow.test.ts index 8a7dfaf10e..adc1a76723 100644 --- a/test/sweep-workflow.test.ts +++ b/test/sweep-workflow.test.ts @@ -3647,6 +3647,7 @@ test("exact PR reviews fail closed unless the leased source head is checked out" "${{ fromJSON(steps.claim-exact-review-queue.outputs.decision).sourceHeadSha || '' }}", ); assert.match(checkout.run ?? "", /repair:exact-review-source/); + assert.match(checkout.run ?? "", /--base-branch "\$target_branch"/); assert.equal( requeue.if, "${{ steps.claim-exact-review-queue.outputs.claimed == 'true' && steps.checkout-target.outputs.source_drift == 'true' }}",