From 5d89a704d99aefaf1c6991175860de526b5cc620 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 28 Jul 2026 10:16:38 -0700 Subject: [PATCH] feat(tooling): repin PR review artifacts --- scripts/pr | 10 +- scripts/pr-lib/review-artifacts.mjs | 170 +++++++++++++++++- scripts/pr-lib/review.sh | 24 +++ .../pr-review-artifact-validation.test.ts | 133 ++++++++++++++ test/scripts/pr-wrappers.test.ts | 2 + 5 files changed, 335 insertions(+), 4 deletions(-) diff --git a/scripts/pr b/scripts/pr index 50a5e5ad2bbc..8705eea77135 100755 --- a/scripts/pr +++ b/scripts/pr @@ -20,7 +20,7 @@ pr_subcommand_classification() { ls | ci-dispatch) printf 'advisory\n' ;; - gc | lock-recover | review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | review-tests | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | merge-verify | merge-run) + gc | lock-recover | review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-artifacts-repin | review-validate-artifacts | review-tests | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | merge-verify | merge-run) printf 'landing\n' ;; *) return 1 ;; @@ -178,6 +178,7 @@ Usage: scripts/pr review-claim scripts/pr review-guard scripts/pr review-artifacts-init + scripts/pr review-artifacts-repin scripts/pr review-validate-artifacts scripts/pr review-tests [ ...] scripts/pr prepare-init @@ -293,7 +294,7 @@ main() { exit 2 fi ;; - review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | ci-dispatch | merge-verify) + review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-artifacts-repin | review-validate-artifacts | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | ci-dispatch | merge-verify) [ "$#" -ge 1 ] || { usage; exit 2; } ;; *) @@ -359,6 +360,11 @@ main() { [ -n "$pr" ] || { usage; exit 2; } review_artifacts_init "$pr" ;; + review-artifacts-repin) + local pr="${1-}" + [ -n "$pr" ] || { usage; exit 2; } + review_artifacts_repin "$pr" + ;; review-validate-artifacts) local pr="${1-}" [ -n "$pr" ] || { usage; exit 2; } diff --git a/scripts/pr-lib/review-artifacts.mjs b/scripts/pr-lib/review-artifacts.mjs index 1a10f2a42363..fe485b24d9f2 100644 --- a/scripts/pr-lib/review-artifacts.mjs +++ b/scripts/pr-lib/review-artifacts.mjs @@ -1,6 +1,7 @@ #!/usr/bin/env node -import { readFileSync } from "node:fs"; +import { existsSync, mkdtempSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; +import { dirname, join } from "node:path"; import { isDirectRunUrl } from "../lib/direct-run.mjs"; const REVIEW_ARTIFACT_ENUMS = Object.freeze({ @@ -533,6 +534,154 @@ function readJson(filePath) { } } +function skipJsonWhitespace(source, start) { + let index = start; + while (/\s/u.test(source[index] ?? "")) { + index += 1; + } + return index; +} + +function jsonStringEnd(source, start) { + for (let index = start + 1; index < source.length; index += 1) { + if (source[index] === "\\") { + index += 1; + } else if (source[index] === '"') { + return index + 1; + } + } + throw new Error("Invalid JSON string while locating review identity"); +} + +function jsonValueEnd(source, start) { + const first = source[start]; + if (first === '"') { + return jsonStringEnd(source, start); + } + if (first === "{" || first === "[") { + const openings = [first]; + for (let index = start + 1; index < source.length; index += 1) { + const character = source[index]; + if (character === '"') { + index = jsonStringEnd(source, index) - 1; + } else if (character === "{" || character === "[") { + openings.push(character); + } else if (character === "}" || character === "]") { + openings.pop(); + if (openings.length === 0) { + return index + 1; + } + } + } + throw new Error("Invalid JSON container while locating review identity"); + } + let index = start; + while (index < source.length && !/[\s,}\]]/u.test(source[index])) { + index += 1; + } + return index; +} + +function findJsonPropertyValue(source, objectStart, property) { + let index = skipJsonWhitespace(source, objectStart + 1); + let found; + while (source[index] !== "}") { + const keyStart = index; + const keyEnd = jsonStringEnd(source, keyStart); + const key = JSON.parse(source.slice(keyStart, keyEnd)); + index = skipJsonWhitespace(source, keyEnd); + if (source[index] !== ":") { + throw new Error("Invalid JSON object while locating review identity"); + } + const valueStart = skipJsonWhitespace(source, index + 1); + const valueEnd = jsonValueEnd(source, valueStart); + if (key === property) { + found = { start: valueStart, end: valueEnd }; + } + index = skipJsonWhitespace(source, valueEnd); + if (source[index] === ",") { + index = skipJsonWhitespace(source, index + 1); + } + } + if (found) { + return found; + } + throw new Error(`Missing ${property} while locating review identity`); +} + +function repinReviewArtifacts({ expectedPrNumber, reviewPath, reviewMarkdownPath, prMetaPath }) { + if (!existsSync(prMetaPath)) { + throw new Error(`Missing ${prMetaPath}; run scripts/pr review-init .`); + } + for (const artifactPath of [reviewPath, reviewMarkdownPath]) { + if (!existsSync(artifactPath)) { + throw new Error(`Missing ${artifactPath}; run scripts/pr review-artifacts-init .`); + } + } + + const prMeta = readJson(prMetaPath); + if ( + !isObject(prMeta) || + prMeta.number !== expectedPrNumber || + typeof prMeta.headRefOid !== "string" || + !/^[0-9a-f]{40}$/u.test(prMeta.headRefOid) + ) { + throw new Error( + `Invalid PR identity in ${prMetaPath}; run scripts/pr review-init ${expectedPrNumber}.`, + ); + } + + const reviewSource = readFileSync(reviewPath, "utf8"); + const review = readJson(reviewPath); + if (!isObject(review) || !isObject(review.pr)) { + throw new Error( + `Invalid PR identity in ${reviewPath}; run scripts/pr review-artifacts-init ${expectedPrNumber}.`, + ); + } + const objectStart = skipJsonWhitespace(reviewSource, 0); + const prSpan = findJsonPropertyValue(reviewSource, objectStart, "pr"); + const numberSpan = findJsonPropertyValue(reviewSource, prSpan.start, "number"); + const headShaSpan = findJsonPropertyValue(reviewSource, prSpan.start, "headSha"); + const replacements = [ + { ...numberSpan, value: String(prMeta.number) }, + { ...headShaSpan, value: JSON.stringify(prMeta.headRefOid) }, + ].toSorted((left, right) => right.start - left.start); + let nextReviewSource = reviewSource; + for (const replacement of replacements) { + nextReviewSource = + nextReviewSource.slice(0, replacement.start) + + replacement.value + + nextReviewSource.slice(replacement.end); + } + + const markdownSource = readFileSync(reviewMarkdownPath, "utf8"); + const firstNewline = markdownSource.indexOf("\n"); + const currentIdentityLine = + firstNewline === -1 ? markdownSource : markdownSource.slice(0, firstNewline); + if (!/^Review artifact for PR #[1-9][0-9]* at [0-9a-f]{40}$/u.test(currentIdentityLine)) { + throw new Error( + `Invalid review identity line in ${reviewMarkdownPath}; run scripts/pr review-artifacts-init ${expectedPrNumber}.`, + ); + } + const markdownBody = firstNewline === -1 ? "" : markdownSource.slice(firstNewline + 1); + const nextMarkdownSource = `${reviewIdentityLine({ + number: prMeta.number, + headSha: prMeta.headRefOid, + })}${firstNewline === -1 ? "" : `\n${markdownBody}`}`; + + const temporaryDir = mkdtempSync(join(dirname(reviewPath), ".review-artifacts-repin-")); + try { + const temporaryReviewPath = join(temporaryDir, "review.json"); + const temporaryMarkdownPath = join(temporaryDir, "review.md"); + writeFileSync(temporaryReviewPath, nextReviewSource, "utf8"); + writeFileSync(temporaryMarkdownPath, nextMarkdownSource, "utf8"); + renameSync(temporaryReviewPath, reviewPath); + renameSync(temporaryMarkdownPath, reviewMarkdownPath); + } finally { + rmSync(temporaryDir, { recursive: true, force: true }); + } +} + function main(argv = process.argv.slice(2)) { const [command, ...args] = argv; if ((command === "template" || command === "markdown") && args.length === 2) { @@ -568,8 +717,25 @@ function main(argv = process.argv.slice(2)) { } return; } + if (command === "repin" && args.length === 4) { + const [prNumber, reviewPath, reviewMarkdownPath, prMetaPath] = args; + if (!/^[1-9][0-9]*$/u.test(prNumber)) { + console.error( + "Usage: review-artifacts.mjs repin ", + ); + process.exitCode = 2; + return; + } + repinReviewArtifacts({ + expectedPrNumber: Number(prNumber), + reviewPath, + reviewMarkdownPath, + prMetaPath, + }); + return; + } console.error( - "Usage: review-artifacts.mjs template|markdown | validate ", + "Usage: review-artifacts.mjs template|markdown | validate | repin ", ); process.exitCode = 2; } diff --git a/scripts/pr-lib/review.sh b/scripts/pr-lib/review.sh index 550ca08b06d5..d9d7d6ab3ff3 100644 --- a/scripts/pr-lib/review.sh +++ b/scripts/pr-lib/review.sh @@ -207,6 +207,30 @@ review_artifacts_init() { echo "files=.local/review.md .local/review.json" } +review_artifacts_repin() { + local pr="$1" + enter_worktree "$pr" false + if [ ! -f .local/pr-meta.json ]; then + echo "Missing .local/pr-meta.json; run scripts/pr review-init $pr." + return 1 + fi + local artifact + for artifact in .local/review.json .local/review.md; do + if [ ! -f "$artifact" ]; then + echo "Missing $artifact; run scripts/pr review-artifacts-init $pr." + return 1 + fi + done + + mark_pr_operation_side_effects_started + node "$(review_artifacts_helper_path)" repin \ + "$pr" \ + .local/review.json \ + .local/review.md \ + .local/pr-meta.json + echo "review artifacts repinned to PR #$pr identity from .local/pr-meta.json" +} + validate_review_artifact_data() { # pr-meta.json is the identity authority the review artifacts are stamped against, # so it must itself be anchored: review_guard binds pr-meta.env to the guarded PR diff --git a/test/scripts/pr-review-artifact-validation.test.ts b/test/scripts/pr-review-artifact-validation.test.ts index e90ca6601418..f53dd73e3cd5 100644 --- a/test/scripts/pr-review-artifact-validation.test.ts +++ b/test/scripts/pr-review-artifact-validation.test.ts @@ -12,6 +12,8 @@ const describePosix = process.platform === "win32" ? describe.skip : describe; const REVIEWED_PR = 42; const REVIEWED_HEAD = "b".repeat(40); +const SUPERSEDED_PR = 7; +const SUPERSEDED_HEAD = "a".repeat(40); const REVIEWED_IDENTITY_LINE = `Review artifact for PR #${REVIEWED_PR} at ${REVIEWED_HEAD}`; const REVIEW_SHELL_COMMAND_SURFACE = [ "rg() {", @@ -184,6 +186,77 @@ function runArtifactsInit(existing: { review?: unknown; markdown?: string } = {} return { result, localDir }; } +function runArtifactsRepin( + options: { + invalidMarkdownIdentity?: boolean; + missing?: "review.json" | "review.md" | "pr-meta.json"; + } = {}, +) { + const fixtureRoot = tempDirs.make("openclaw-pr-review-artifacts-repin-"); + const localDir = join(fixtureRoot, ".local"); + mkdirSync(localDir); + const review = validReadyReview(); + review.pr.number = SUPERSEDED_PR; + review.pr.headSha = SUPERSEDED_HEAD; + review.findings.push({ + id: "authored-nit", + title: "Preserve this authored finding", + area: "tooling", + fix: "Keep the nested authored content byte-identical.", + severity: "NIT", + }); + review.nitSweep.status = "has_nits"; + review.issueValidation.summary = "Authored nested summary."; + const reviewSource = `${JSON.stringify(review, null, 4)}\n`; + const markdownBody = [ + "", + "A) Authored recommendation", + "", + "B) Authored details stay byte-identical.", + "", + "C) Security", + "D) Intent", + "E) Concerns", + "F) Tests", + "G) Docs", + "H) Changelog", + "I) Follow ups", + "J) Suggested comment", + "", + ].join("\n"); + const markdownSource = options.invalidMarkdownIdentity + ? `A) Authored recommendation without an identity header\n${markdownBody}` + : `Review artifact for PR #${SUPERSEDED_PR} at ${SUPERSEDED_HEAD}\n${markdownBody}`; + const prMetaSource = `${JSON.stringify({ + number: REVIEWED_PR, + headRefOid: REVIEWED_HEAD, + files: [], + })}\n`; + const sources = { + "review.json": reviewSource, + "review.md": markdownSource, + "pr-meta.json": prMetaSource, + }; + for (const [name, source] of Object.entries(sources)) { + if (name !== options.missing) { + writeFileSync(join(localDir, name), source); + } + } + const result = spawnSync( + process.execPath, + [ + reviewArtifactsScript, + "repin", + String(REVIEWED_PR), + join(localDir, "review.json"), + join(localDir, "review.md"), + join(localDir, "pr-meta.json"), + ], + { encoding: "utf8" }, + ); + return { result, localDir, reviewSource, markdownBody }; +} + function runMergeVerification(checks: "api-error" | "invalid-json" | "no-required" | "pending") { const fixtureRoot = tempDirs.make("openclaw-pr-merge-verification-"); const localDir = join(fixtureRoot, ".local"); @@ -360,6 +433,66 @@ describePosix("scripts/pr review artifact validation", () => { ).toBe("Half-written review worth keeping."); }); + it("repins both artifacts without changing authored content", () => { + const { result, localDir, reviewSource, markdownBody } = runArtifactsRepin(); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + const repinnedReview = readFileSync(join(localDir, "review.json"), "utf8"); + const expectedReview = reviewSource + .replace(`"number": ${SUPERSEDED_PR}`, `"number": ${REVIEWED_PR}`) + .replace(`"headSha": "${SUPERSEDED_HEAD}"`, `"headSha": "${REVIEWED_HEAD}"`); + expect(repinnedReview).toBe(expectedReview); + expect(readFileSync(join(localDir, "review.md"), "utf8")).toBe( + `${REVIEWED_IDENTITY_LINE}\n${markdownBody}`, + ); + + const validation = spawnSync( + process.execPath, + [ + reviewArtifactsScript, + "validate", + join(localDir, "review.json"), + join(localDir, "review.md"), + join(localDir, "pr-meta.json"), + ], + { encoding: "utf8" }, + ); + expect(validation.status, `${validation.stdout}\n${validation.stderr}`).toBe(0); + }); + + it.each([ + { + missing: "review.json" as const, + message: "review.json; run scripts/pr review-artifacts-init ", + }, + { + missing: "review.md" as const, + message: "review.md; run scripts/pr review-artifacts-init ", + }, + { + missing: "pr-meta.json" as const, + message: "pr-meta.json; run scripts/pr review-init ", + }, + ])("refuses repin when $missing is missing", ({ missing, message }) => { + const { result } = runArtifactsRepin({ missing }); + + expect(result.status).toBe(1); + expect(result.stdout).toContain(message); + }); + + it("refuses to discard a malformed Markdown first line", () => { + const { result, localDir } = runArtifactsRepin({ invalidMarkdownIdentity: true }); + const markdownPath = join(localDir, "review.md"); + + expect(result.status).toBe(1); + expect(result.stdout).toContain( + `Invalid review identity line in ${markdownPath}; run scripts/pr review-artifacts-init ${REVIEWED_PR}.`, + ); + expect(readFileSync(markdownPath, "utf8")).toContain( + "A) Authored recommendation without an identity header", + ); + }); + it("rejects a review guard whose PR metadata describes another PR", () => { const fixtureRoot = tempDirs.make("openclaw-pr-review-guard-"); const localDir = join(fixtureRoot, ".local"); diff --git a/test/scripts/pr-wrappers.test.ts b/test/scripts/pr-wrappers.test.ts index 4529bdc1eaff..f384481bdf9a 100644 --- a/test/scripts/pr-wrappers.test.ts +++ b/test/scripts/pr-wrappers.test.ts @@ -168,12 +168,14 @@ describe("scripts/pr wrappers", () => { expect(script).toContain("OPENCLAW_GH_BIN="); expect(script).toContain("gh_plain"); expect(script).toContain("scripts/pr review-init "); + expect(script).toContain("scripts/pr review-artifacts-repin "); expect(script).toContain("scripts/pr prepare-run "); expect(script).toContain("scripts/pr ci-dispatch "); expect(script).toContain("scripts/pr merge-run [--auto-merge]"); expect(script).toContain("OPENCLAW_PR_AUTO_MERGE=1 is equivalent"); expect(script).toContain("Required commands: git, gh, jq, rg (ripgrep), pnpm, node."); expect(script).toContain('review_init "$pr"'); + expect(script).toContain('review_artifacts_repin "$pr"'); expect(script).toContain('prepare_run "$pr"'); expect(script).toContain('ci_dispatch "$pr"'); expect(script).toContain('merge_run "$pr" "$auto_merge"');