diff --git a/scripts/pr b/scripts/pr index ea1d1e6395cb..2d9572fbb7bc 100755 --- a/scripts/pr +++ b/scripts/pr @@ -240,8 +240,9 @@ require_cmds() { require_main_target_pr() { local pr="$1" - local base - base=$(gh pr view "$pr" --json baseRefName --jq .baseRefName) + local base base_json + base_json=$(read_pr_view_json "$pr" "baseRefName") || exit 1 + base=$(pr_view_string_field "$base_json" "baseRefName" "$pr" "Retry the scripts/pr command.") || exit 1 if [ "$base" != "main" ]; then echo "scripts/pr prepare and merge commands only support PRs targeting main; PR #$pr targets $base." >&2 echo "Use the reviewed release-branch landing flow for non-main PRs." >&2 diff --git a/scripts/pr-lib/common.sh b/scripts/pr-lib/common.sh index 26bfa06c9d1f..3ecc81edaef4 100644 --- a/scripts/pr-lib/common.sh +++ b/scripts/pr-lib/common.sh @@ -145,6 +145,60 @@ bootstrap_deps_if_needed() { fi } +read_pr_view_json() { + local pr="$1" + local fields="$2" + local max_attempts=3 + local temp_dir + temp_dir=$(mktemp -d "${TMPDIR:-/tmp}/openclaw-pr-view.XXXXXX") || { + echo "Unable to create temporary storage for GitHub PR metadata." >&2 + return 1 + } + local stdout_file="$temp_dir/stdout" + local stderr_file="$temp_dir/stderr" + local attempt exit_code reason + + for attempt in $(seq 1 "$max_attempts"); do + exit_code=0 + if gh pr view "$pr" --json "$fields" >"$stdout_file" 2>"$stderr_file"; then + if [ -s "$stdout_file" ] && jq -se 'length == 1 and (.[0] | type == "object")' "$stdout_file" >/dev/null 2>&1; then + cat "$stdout_file" + rm -rf "$temp_dir" + return 0 + fi + if [ ! -s "$stdout_file" ]; then + reason="gh pr view returned empty stdout" + else + reason="gh pr view did not return one JSON object" + fi + else + exit_code=$? + reason="gh pr view exited with status $exit_code" + fi + [ "$attempt" -eq "$max_attempts" ] || sleep "$attempt" + done + + echo "GitHub API failure while reading PR #$pr: $reason after $max_attempts attempts." >&2 + [ ! -s "$stderr_file" ] || cat "$stderr_file" >&2 + rm -rf "$temp_dir" + return 1 +} + +pr_view_string_field() { + local json="$1" field="$2" pr="$3" remedy="${4:-Retry the command.}" label value + case "$field" in + headRefOid) label="a head SHA" ;; + baseRefName) label="a base branch" ;; + headRefName) label="a head branch" ;; + *) label="a non-empty .$field string" ;; + esac + if ! value=$(printf '%s\n' "$json" | jq -er --arg field "$field" '.[$field] | if type == "string" and length > 0 then . else error("missing string field") end' 2>/dev/null); then + echo "GitHub PR metadata for #$pr did not include $label. $remedy" >&2 + return 1 + fi + printf '%s\n' "$value" +} + wait_for_pr_head_sha() { local pr="$1" local expected_sha="$2" diff --git a/scripts/pr-lib/gates.sh b/scripts/pr-lib/gates.sh index 14ba8b499eaf..677e07aae9d9 100644 --- a/scripts/pr-lib/gates.sh +++ b/scripts/pr-lib/gates.sh @@ -4,8 +4,8 @@ run_hosted_prepare_gates() { local changelog_only="$3" local recent_sha="" local remote_record remote_head remote_head_ref remote_is_cross_repository - remote_record=$(gh pr view "$pr" --json headRefName,headRefOid,isCrossRepository) - remote_head=$(printf '%s\n' "$remote_record" | jq -r .headRefOid) + remote_record=$(read_pr_view_json "$pr" "headRefName,headRefOid,isCrossRepository") || return 1 + remote_head=$(pr_view_string_field "$remote_record" "headRefOid" "$pr" "Re-run prepare-init.") || return 1 remote_head_ref=$(printf '%s\n' "$remote_record" | jq -r .headRefName) remote_is_cross_repository=$(printf '%s\n' "$remote_record" | jq -r .isCrossRepository) if [ "$remote_head" != "$current_head" ]; then diff --git a/scripts/pr-lib/merge.sh b/scripts/pr-lib/merge.sh index 4f33c4852bc9..290a0f254909 100644 --- a/scripts/pr-lib/merge.sh +++ b/scripts/pr-lib/merge.sh @@ -242,7 +242,7 @@ merge_run() { source .local/prep.env local pr_meta_json - pr_meta_json=$(gh pr view "$pr" --json state,isDraft) + pr_meta_json=$(read_pr_view_json "$pr" "state,isDraft") || exit 1 local is_draft is_draft=$(printf '%s\n' "$pr_meta_json" | jq -r .isDraft) if [ "$is_draft" = "true" ]; then @@ -312,7 +312,7 @@ merge_run() { # artifact, exact-head, required-check, and drift check in merge_verify. if [ "$auto_merge_requested" = "true" ]; then local auto_meta - auto_meta=$(gh pr view "$pr" --json state,headRefOid,mergeable,mergeStateStatus,autoMergeRequest) + auto_meta=$(read_pr_view_json "$pr" "state,headRefOid,mergeable,mergeStateStatus,autoMergeRequest") || exit 1 local auto_head_sha auto_head_sha=$(printf '%s\n' "$auto_meta" | jq -r .headRefOid) if [ "$auto_head_sha" != "$PREP_HEAD_SHA" ]; then @@ -338,7 +338,7 @@ merge_run() { print_relevant_log_excerpt .local/merge-output.log exit 1 fi - auto_meta=$(gh pr view "$pr" --json state,headRefOid,mergeable,mergeStateStatus,autoMergeRequest) + auto_meta=$(read_pr_view_json "$pr" "state,headRefOid,mergeable,mergeStateStatus,autoMergeRequest") || exit 1 auto_head_sha=$(printf '%s\n' "$auto_meta" | jq -r .headRefOid) mergeable=$(printf '%s\n' "$auto_meta" | jq -r '.mergeable // "UNKNOWN"') merge_state_status=$(printf '%s\n' "$auto_meta" | jq -r '.mergeStateStatus // "UNKNOWN"') @@ -365,7 +365,7 @@ merge_run() { --match-head-commit "$PREP_HEAD_SHA" \ >.local/merge-output.log 2>&1 then - auto_meta=$(gh pr view "$pr" --json state,headRefOid,mergeable,mergeStateStatus,autoMergeRequest) + auto_meta=$(read_pr_view_json "$pr" "state,headRefOid,mergeable,mergeStateStatus,autoMergeRequest") || exit 1 auto_head_sha=$(printf '%s\n' "$auto_meta" | jq -r .headRefOid) state=$(printf '%s\n' "$auto_meta" | jq -r .state) existing_auto_method=$(printf '%s\n' "$auto_meta" | jq -r '.autoMergeRequest.mergeMethod // ""') @@ -385,7 +385,7 @@ merge_run() { exit 1 fi else - auto_meta=$(gh pr view "$pr" --json state,headRefOid,autoMergeRequest) + auto_meta=$(read_pr_view_json "$pr" "state,headRefOid,autoMergeRequest") || exit 1 auto_head_sha=$(printf '%s\n' "$auto_meta" | jq -r .headRefOid) existing_auto_method=$(printf '%s\n' "$auto_meta" | jq -r '.autoMergeRequest.mergeMethod // ""') if [ "$auto_head_sha" = "$PREP_HEAD_SHA" ] && [ -n "$existing_auto_method" ]; then diff --git a/scripts/pr-lib/push.sh b/scripts/pr-lib/push.sh index 04f719424c62..67dce01c49a1 100644 --- a/scripts/pr-lib/push.sh +++ b/scripts/pr-lib/push.sh @@ -179,8 +179,9 @@ verify_pr_head_branch_matches_expected() { local pr="$1" local expected_head="$2" - local current_head - current_head=$(gh pr view "$pr" --json headRefName --jq .headRefName) + local current_head current_head_json + current_head_json=$(read_pr_view_json "$pr" "headRefName") || exit 1 + current_head=$(pr_view_string_field "$current_head_json" "headRefName" "$pr" "Re-run prepare-init.") || exit 1 if [ "$current_head" != "$expected_head" ]; then echo "PR head branch changed from $expected_head to $current_head. Re-run prepare-init." exit 1 diff --git a/scripts/pr-lib/worktree.sh b/scripts/pr-lib/worktree.sh index d32f0cbe78ce..916d2d341923 100644 --- a/scripts/pr-lib/worktree.sh +++ b/scripts/pr-lib/worktree.sh @@ -268,10 +268,13 @@ enter_worktree() { pr_meta_json() { local pr="$1" - local metadata files expected_file_count actual_file_count head_before head_after - metadata=$(gh pr view "$pr" --json number,title,state,isDraft,author,baseRefName,headRefName,headRefOid,headRepository,headRepositoryOwner,url,body,labels,assignees,changedFiles,additions,deletions,statusCheckRollup,files) - head_before=$(printf '%s\n' "$metadata" | jq -r .headRefOid) - expected_file_count=$(printf '%s\n' "$metadata" | jq -r .changedFiles) + local metadata files expected_file_count actual_file_count head_before head_after head_after_json + metadata=$(read_pr_view_json "$pr" "number,title,state,isDraft,author,baseRefName,headRefName,headRefOid,headRepository,headRepositoryOwner,url,body,labels,assignees,changedFiles,additions,deletions,statusCheckRollup,files") || return 1 + head_before=$(pr_view_string_field "$metadata" "headRefOid" "$pr" "Retry review initialization.") || return 1 + if ! expected_file_count=$(printf '%s\n' "$metadata" | jq -er '.changedFiles | if type == "number" and . >= 0 and . == floor then . else error("invalid changed file count") end' 2>/dev/null); then + echo "Invalid PR metadata for #$pr: changedFiles must be a non-negative integer." >&2 + return 1 + fi # `gh pr view --json files` is cacheable but stops at 100 entries. Use it # when complete; only large or incomplete responses spend uncached REST quota. @@ -327,7 +330,8 @@ pr_meta_json() { fi fi - head_after=$(gh pr view "$pr" --json headRefOid | jq -r .headRefOid) + head_after_json=$(read_pr_view_json "$pr" "headRefOid") || return 1 + head_after=$(pr_view_string_field "$head_after_json" "headRefOid" "$pr" "Retry review initialization.") || return 1 if [ "$head_after" != "$head_before" ]; then echo "PR head changed while collecting file metadata for #$pr (started at $head_before, ended at $head_after). Retry review initialization." >&2 return 1 diff --git a/test/scripts/pr-merge.test.ts b/test/scripts/pr-merge.test.ts index d94831e8897e..7b9ab85efe69 100644 --- a/test/scripts/pr-merge.test.ts +++ b/test/scripts/pr-merge.test.ts @@ -5,6 +5,7 @@ import { afterEach, describe, expect, it } from "vitest"; import { useAutoCleanupTempDirTracker } from "../helpers/temp-dir.js"; const tempDirs = useAutoCleanupTempDirTracker(afterEach); +const commonScript = join(process.cwd(), "scripts/pr-lib/common.sh"); const mergeScript = join(process.cwd(), "scripts/pr-lib/merge.sh"); const headSha = "0123456789abcdef0123456789abcdef01234567"; const landedSha = "fedcba9876543210fedcba9876543210fedcba98"; @@ -89,6 +90,7 @@ process.exit(new RegExp(pattern, flags).test(readFileSync(file, "utf8")) ? 0 : 1 const shell = ` set -euo pipefail +source "$OPENCLAW_TEST_COMMON_SCRIPT" source "$OPENCLAW_TEST_MERGE_SCRIPT" script_parent_dir="$OPENCLAW_TEST_SCRIPTS_DIR" enter_worktree() { :; } @@ -259,6 +261,7 @@ merge_run 123 "$OPENCLAW_TEST_AUTO_REQUESTED" OPENCLAW_TEST_COMMENT_BODY: commentBody, OPENCLAW_TEST_COMMENT_EMPTY: scenario.commentEmpty ? "true" : "false", OPENCLAW_TEST_COMMENT_FAILURES: String(scenario.commentFailures ?? 0), + OPENCLAW_TEST_COMMON_SCRIPT: commonScript, OPENCLAW_TEST_DISABLED_AUTO_META: disabledAutoMeta, OPENCLAW_TEST_GH_CALLS: calls, OPENCLAW_TEST_LANDED_SHA: landedSha, diff --git a/test/scripts/pr-metadata.test.ts b/test/scripts/pr-metadata.test.ts index 1d8fb0dee17d..0eeba43b9089 100644 --- a/test/scripts/pr-metadata.test.ts +++ b/test/scripts/pr-metadata.test.ts @@ -16,16 +16,41 @@ function createFakeGh(): string { set -euo pipefail if [ "$1" = "pr" ] && [ "$2" = "view" ]; then + pr_view_count=0 + if [ -f "$FAKE_PR_VIEW_COUNT_FILE" ]; then + IFS= read -r pr_view_count < "$FAKE_PR_VIEW_COUNT_FILE" + fi + pr_view_count=$((pr_view_count + 1)) + printf '%s\n' "$pr_view_count" > "$FAKE_PR_VIEW_COUNT_FILE" + + failure_target_matches=0 + if [ "\${FAKE_PR_VIEW_FAILURE_TARGET:-all}" = "all" ] || { + [ "\${FAKE_PR_VIEW_FAILURE_TARGET:-all}" = "head" ] && [[ "$*" != *changedFiles* ]] + }; then + failure_target_matches=1 + fi + if [ -n "\${FAKE_PR_VIEW_FAILURE_MODE:-}" ] && [ "$failure_target_matches" = "1" ] && { + [ "\${FAKE_PR_VIEW_FAILURE_COUNT:--1}" = "-1" ] || [ "$pr_view_count" -le "\${FAKE_PR_VIEW_FAILURE_COUNT}" ] + }; then + echo "HTTP 503: No server is currently available to service your request. (https://api.github.com/graphql)" >&2 + case "$FAKE_PR_VIEW_FAILURE_MODE" in + empty) exit 0 ;; + exit) exit 7 ;; + non-json) printf 'upstream unavailable\n'; exit 0 ;; + null) printf 'null\n'; exit 0 ;; + esac + fi + if [[ "$*" == *changedFiles* ]]; then if [ "\${FAKE_REJECT_REVIEW_REQUESTS:-0}" = "1" ] && [[ "$*" == *reviewRequests* ]]; then echo "GraphQL: Resource not accessible by integration (repository.pullRequest.reviewRequests.nodes.0.requestedReviewer)" >&2 exit 1 fi - jq -nc --argjson changedFiles "\${FAKE_CHANGED_FILES:-101}" --argjson fileCount "\${FAKE_GRAPHQL_FILE_COUNT:-100}" --argjson includeChangeType "\${FAKE_GRAPHQL_CHANGE_TYPE:-true}" ' + jq -nc --arg headRefOid "\${FAKE_HEAD_BEFORE-head-a}" --argjson changedFiles "\${FAKE_CHANGED_FILES:-101}" --argjson fileCount "\${FAKE_GRAPHQL_FILE_COUNT:-100}" --argjson includeChangeType "\${FAKE_GRAPHQL_CHANGE_TYPE:-true}" ' { number: 42, url: "https://example.test/pr/42", - headRefOid: "head-a", + headRefOid: $headRefOid, changedFiles: $changedFiles, files: [ range(0; $fileCount) @@ -77,6 +102,10 @@ function readPrMetadata( graphqlChangeType?: boolean; graphqlFileCount?: string; headAfter?: string; + headBefore?: string; + prViewFailureCount?: string; + prViewFailureMode?: "empty" | "exit" | "non-json" | "null"; + prViewFailureTarget?: "all" | "head"; rejectReviewRequests?: boolean; restFileCount?: string; } = {}, @@ -85,7 +114,7 @@ function readPrMetadata( "bash", [ "-c", - "set -euo pipefail; source scripts/lib/plain-gh.sh; source scripts/pr-lib/worktree.sh; pr_meta_json 42", + "set -euo pipefail; source scripts/lib/plain-gh.sh; source scripts/pr-lib/worktree.sh; source scripts/pr-lib/common.sh; pr_meta_json 42", ], { cwd: process.cwd(), @@ -96,6 +125,11 @@ function readPrMetadata( FAKE_GRAPHQL_CHANGE_TYPE: options.graphqlChangeType === false ? "false" : "true", FAKE_GRAPHQL_FILE_COUNT: options.graphqlFileCount ?? "100", FAKE_HEAD_AFTER: options.headAfter ?? "head-a", + FAKE_HEAD_BEFORE: options.headBefore ?? "head-a", + FAKE_PR_VIEW_COUNT_FILE: join(fakeGhDir, "pr-view-count"), + FAKE_PR_VIEW_FAILURE_COUNT: options.prViewFailureCount ?? "-1", + FAKE_PR_VIEW_FAILURE_MODE: options.prViewFailureMode ?? "", + FAKE_PR_VIEW_FAILURE_TARGET: options.prViewFailureTarget ?? "all", FAKE_REJECT_REVIEW_REQUESTS: options.rejectReviewRequests ? "1" : "0", FAKE_REST_FILE_COUNT: options.restFileCount ?? "101", OPENCLAW_GH_BIN: join(fakeGhDir, "gh"), @@ -216,4 +250,74 @@ describe("PR metadata", () => { "PR head changed while collecting file metadata for #42 (started at head-a, ended at head-b). Retry review initialization.", ); }); + + it("rejects metadata without an observed initial head SHA", () => { + const result = readPrMetadata(createFakeGh(), { + changedFiles: "2", + graphqlFileCount: "2", + headBefore: "", + }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain( + "GitHub PR metadata for #42 did not include a head SHA. Retry review initialization.", + ); + expect(result.stderr).not.toContain("PR head changed"); + }); + + it("rejects a non-numeric changed file count before shell comparison", () => { + const result = readPrMetadata(createFakeGh(), { changedFiles: "null" }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain( + "Invalid PR metadata for #42: changedFiles must be a non-negative integer.", + ); + expect(result.stderr).not.toContain("integer expected"); + expect(result.stderr).not.toContain("integer expression expected"); + }); + + it.each([ + ["empty stdout", "empty", "returned empty stdout"], + ["a non-zero exit", "exit", "exited with status 7"], + ["non-JSON stdout", "non-json", "did not return one JSON object"], + ["a non-object JSON value", "null", "did not return one JSON object"], + ] as const)("reports a GitHub API failure for %s", (_label, prViewFailureMode, detail) => { + const result = readPrMetadata(createFakeGh(), { prViewFailureMode }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain( + `GitHub API failure while reading PR #42: gh pr view ${detail}`, + ); + expect(result.stderr).toContain("HTTP 503: No server is currently available"); + expect(result.stderr).not.toContain("integer expected"); + expect(result.stderr).not.toContain("PR head changed"); + }); + + it("reports an API failure when the post-collection head read fails", () => { + const result = readPrMetadata(createFakeGh(), { + changedFiles: "2", + graphqlFileCount: "2", + prViewFailureMode: "empty", + prViewFailureTarget: "head", + }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain( + "GitHub API failure while reading PR #42: gh pr view returned empty stdout", + ); + expect(result.stderr).not.toContain("PR head changed"); + }); + + it("recovers when a transient API failure is followed by a valid PR object", () => { + const result = readPrMetadata(createFakeGh(), { + changedFiles: "2", + graphqlFileCount: "2", + prViewFailureCount: "1", + prViewFailureMode: "empty", + }); + + expect(result.status).toBe(0); + expect(result.stderr).toBe(""); + expect(JSON.parse(result.stdout)).toMatchObject({ headRefOid: "head-a", changedFiles: 2 }); + }); }); diff --git a/test/scripts/pr-wrappers.test.ts b/test/scripts/pr-wrappers.test.ts index 49f1cbc0ef6c..7364e04d9271 100644 --- a/test/scripts/pr-wrappers.test.ts +++ b/test/scripts/pr-wrappers.test.ts @@ -34,7 +34,7 @@ function makeMismatchedWrapperRepo() { mkdirSync(bin, { recursive: true }); mkdirSync(home, { recursive: true }); // This fixture exercises wrapper trust routing, not the host command inventory. - for (const command of ["jq", "pnpm", "rg"]) { + for (const command of ["pnpm", "rg"]) { const commandPath = join(bin, command); writeFileSync(commandPath, "#!/bin/sh\nexit 0\n"); chmodSync(commandPath, 0o755); @@ -44,7 +44,7 @@ function makeMismatchedWrapperRepo() { const ghStub = join(bin, "gh"); writeFileSync( ghStub, - '#!/bin/sh\nif [ "$1" = "pr" ] && [ "$2" = "view" ]; then\n echo not-main\n exit 0\nfi\nexit 0\n', + '#!/bin/sh\nif [ "$1" = "pr" ] && [ "$2" = "view" ]; then\n printf \'{"baseRefName":"not-main"}\\n\'\n exit 0\nfi\nexit 0\n', ); chmodSync(ghStub, 0o755); @@ -219,13 +219,15 @@ describe("scripts/pr wrappers", () => { it("routes cached reads and writer-sensitive operations through their owning gh seams", () => { const script = readScript("scripts/pr"); + const common = readScript("scripts/pr-lib/common.sh"); const worktree = readScript("scripts/pr-lib/worktree.sh"); const review = readScript("scripts/pr-lib/review.sh"); const push = readScript("scripts/pr-lib/push.sh"); const merge = readScript("scripts/pr-lib/merge.sh"); - expect(script).toContain('base=$(gh pr view "$pr" --json baseRefName --jq .baseRefName)'); - expect(worktree).toContain('metadata=$(gh pr view "$pr" --json'); + expect(script).toContain('base_json=$(read_pr_view_json "$pr" "baseRefName")'); + expect(common).toContain('gh pr view "$pr" --json "$fields"'); + expect(worktree).toContain('metadata=$(read_pr_view_json "$pr"'); expect(worktree).toContain('gh_plain api --paginate "repos/{owner}/{repo}/pulls/$pr/files'); expect(review).toContain("reviewer=$(gh_plain api user --jq .login"); expect(review).toContain('gh_plain pr edit "$pr" --add-assignee "$reviewer"');