mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-05 06:01:41 +00:00
feat(tooling): repin PR review artifacts
This commit is contained in:
Peter Steinberger
10
scripts/pr
10
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 <PR>
|
||||
scripts/pr review-guard <PR>
|
||||
scripts/pr review-artifacts-init <PR>
|
||||
scripts/pr review-artifacts-repin <PR>
|
||||
scripts/pr review-validate-artifacts <PR>
|
||||
scripts/pr review-tests <PR> <test-file> [<test-file> ...]
|
||||
scripts/pr prepare-init <PR>
|
||||
@@ -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; }
|
||||
|
||||
@@ -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 <PR>.`);
|
||||
}
|
||||
for (const artifactPath of [reviewPath, reviewMarkdownPath]) {
|
||||
if (!existsSync(artifactPath)) {
|
||||
throw new Error(`Missing ${artifactPath}; run scripts/pr review-artifacts-init <PR>.`);
|
||||
}
|
||||
}
|
||||
|
||||
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 <pr-number> <review.json> <review.md> <pr-meta.json>",
|
||||
);
|
||||
process.exitCode = 2;
|
||||
return;
|
||||
}
|
||||
repinReviewArtifacts({
|
||||
expectedPrNumber: Number(prNumber),
|
||||
reviewPath,
|
||||
reviewMarkdownPath,
|
||||
prMetaPath,
|
||||
});
|
||||
return;
|
||||
}
|
||||
console.error(
|
||||
"Usage: review-artifacts.mjs template|markdown <pr-number> <head-sha> | validate <review.json> <review.md> <pr-meta.json>",
|
||||
"Usage: review-artifacts.mjs template|markdown <pr-number> <head-sha> | validate <review.json> <review.md> <pr-meta.json> | repin <pr-number> <review.json> <review.md> <pr-meta.json>",
|
||||
);
|
||||
process.exitCode = 2;
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 <PR>",
|
||||
},
|
||||
{
|
||||
missing: "review.md" as const,
|
||||
message: "review.md; run scripts/pr review-artifacts-init <PR>",
|
||||
},
|
||||
{
|
||||
missing: "pr-meta.json" as const,
|
||||
message: "pr-meta.json; run scripts/pr review-init <PR>",
|
||||
},
|
||||
])("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");
|
||||
|
||||
@@ -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 <PR>");
|
||||
expect(script).toContain("scripts/pr review-artifacts-repin <PR>");
|
||||
expect(script).toContain("scripts/pr prepare-run <PR>");
|
||||
expect(script).toContain("scripts/pr ci-dispatch <PR>");
|
||||
expect(script).toContain("scripts/pr merge-run <PR> [--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"');
|
||||
|
||||
Reference in New Issue
Block a user