diff --git a/scripts/pr-lib/merge.sh b/scripts/pr-lib/merge.sh index 6ba06e6913fb..bb2cca308dd3 100644 --- a/scripts/pr-lib/merge.sh +++ b/scripts/pr-lib/merge.sh @@ -478,30 +478,35 @@ merge_run() { fi local ok=0 - local comment_output="" + local comment_body + printf -v comment_body \ + 'Merged via %s.\n\n- Prepared head SHA: [%s](%s)\n- Landed commit: [%s](%s)' \ + "$merge_label" \ + "$PREP_HEAD_SHA" \ + "$prep_sha_url" \ + "$landed_sha" \ + "$landed_sha_url" + local comment_url="" + local comment_err_file + comment_err_file=$(mktemp) local attempt for attempt in 1 2 3; do - if comment_output=$( - { - echo "Merged via $merge_label." - echo - echo "- Prepared head SHA: [$PREP_HEAD_SHA]($prep_sha_url)" - echo "- Landed commit: [$landed_sha]($landed_sha_url)" - } | gh_plain pr comment "$pr" -F - 2>&1 - ); then + if comment_url=$( + gh_plain api \ + --method POST \ + "repos/{owner}/{repo}/issues/$pr/comments" \ + --raw-field "body=$comment_body" \ + --jq '.html_url // empty' \ + 2>"$comment_err_file" + ) && [ -n "$comment_url" ]; then ok=1 break fi sleep 2 done + rm -f "$comment_err_file" [ "$ok" -eq 1 ] || { echo "Failed to post PR comment after retries"; exit 1; } - local comment_url="" - comment_url=$(printf '%s\n' "$comment_output" | rg -o 'https://github.com/[^ ]+/pull/[0-9]+#issuecomment-[0-9]+' -m1 || true) - if [ -z "$comment_url" ]; then - comment_url="unresolved" - fi - local root root=$(repo_root) cd "$root" diff --git a/test/scripts/pr-merge.test.ts b/test/scripts/pr-merge.test.ts index 88f9c2dd52a2..1a5426473245 100644 --- a/test/scripts/pr-merge.test.ts +++ b/test/scripts/pr-merge.test.ts @@ -15,6 +15,8 @@ type MergeScenario = { autoError?: string; autoResult?: "enabled" | "inconclusive" | "unavailable"; checks?: "fail" | "green" | "pending"; + commentEmpty?: boolean; + commentFailures?: number; existingAutoMethod?: "" | "MERGE" | "REBASE" | "SQUASH"; mergeStateStatus?: string; mergeable?: string; @@ -29,6 +31,9 @@ function runMerge(scenario: MergeScenario = {}) { const autoCalled = join(root, "auto-called"); const autoState = join(root, "auto-state"); const bin = join(root, "bin"); + const commentAttempts = join(root, "comment-attempts"); + const commentBody = join(root, "comment-body"); + const lifecycle = join(root, "lifecycle.log"); const rgCalls = join(root, "rg-calls.log"); mkdirSync(bin, { recursive: true }); mkdirSync(localDir, { recursive: true }); @@ -105,8 +110,9 @@ mark_pr_operation_side_effects_started() { :; } mainline_drift_requires_sync() { return 1; } print_relevant_log_excerpt() { cat "$1"; } repo_root() { printf '%s\\n' "$OPENCLAW_TEST_ROOT"; } -remove_worktree_if_present() { :; } -delete_local_branch_if_safe() { :; } +remove_worktree_if_present() { printf 'worktree-cleanup %s\\n' "$*" >> "$OPENCLAW_TEST_LIFECYCLE"; } +delete_local_branch_if_safe() { printf 'branch-cleanup %s\\n' "$*" >> "$OPENCLAW_TEST_LIFECYCLE"; } +sleep() { :; } pr_meta_json() { printf '%s\\n' '{"state":"OPEN","isDraft":false,"headRefOid":"${headSha}"}' } @@ -190,8 +196,35 @@ gh_route() { esac ;; "repo view") printf 'openclaw/openclaw\\n' ;; - "pr comment") printf 'https://github.com/openclaw/openclaw/pull/123#issuecomment-1\\n' ;; - "api "*) : ;; + "api "*) + case "$*" in + *"issues/123/comments"*) + local arg + for arg in "$@"; do + case "$arg" in + body=*) printf '%s' "\${arg#body=}" > "$OPENCLAW_TEST_COMMENT_BODY" ;; + esac + done + local attempts=0 + if [ -e "$OPENCLAW_TEST_COMMENT_ATTEMPTS" ]; then + attempts=$(cat "$OPENCLAW_TEST_COMMENT_ATTEMPTS") + fi + attempts=$((attempts + 1)) + printf '%s\\n' "$attempts" > "$OPENCLAW_TEST_COMMENT_ATTEMPTS" + printf 'comment\\n' >> "$OPENCLAW_TEST_LIFECYCLE" + if [ "$attempts" -le "$OPENCLAW_TEST_COMMENT_FAILURES" ]; then + echo 'transient comment failure' >&2 + return 1 + fi + if [ "$OPENCLAW_TEST_COMMENT_EMPTY" = "true" ]; then + return 0 + fi + printf 'https://github.com/openclaw/openclaw/pull/123#issuecomment-1\\n' + ;; + *"git/refs/"*) printf 'remote-cleanup\\n' >> "$OPENCLAW_TEST_LIFECYCLE" ;; + *) : ;; + esac + ;; *) echo "unexpected gh invocation: $*" >&2; return 2 ;; esac } @@ -213,9 +246,14 @@ merge_run 123 "$OPENCLAW_TEST_AUTO_REQUESTED" OPENCLAW_TEST_AUTO_STATE: autoState, OPENCLAW_TEST_CHECKS_EXIT_STATUS: scenario.checks === "pending" ? "8" : "0", OPENCLAW_TEST_CHECKS_JSON: JSON.stringify(checks), + OPENCLAW_TEST_COMMENT_ATTEMPTS: commentAttempts, + OPENCLAW_TEST_COMMENT_BODY: commentBody, + OPENCLAW_TEST_COMMENT_EMPTY: scenario.commentEmpty ? "true" : "false", + OPENCLAW_TEST_COMMENT_FAILURES: String(scenario.commentFailures ?? 0), OPENCLAW_TEST_DISABLED_AUTO_META: disabledAutoMeta, OPENCLAW_TEST_GH_CALLS: calls, OPENCLAW_TEST_LANDED_SHA: landedSha, + OPENCLAW_TEST_LIFECYCLE: lifecycle, OPENCLAW_TEST_MERGE_SCRIPT: mergeScript, OPENCLAW_TEST_MERGE_STATE_STATUS: scenario.mergeStateStatus ?? "BEHIND", OPENCLAW_TEST_POST_AUTO_META: postAutoMeta, @@ -231,6 +269,11 @@ merge_run 123 "$OPENCLAW_TEST_AUTO_REQUESTED" return { ...result, calls: existsSync(calls) ? readFileSync(calls, "utf8") : "", + commentAttempts: existsSync(commentAttempts) + ? Number(readFileSync(commentAttempts, "utf8").trim()) + : 0, + commentBody: existsSync(commentBody) ? readFileSync(commentBody, "utf8") : "", + lifecycle: existsSync(lifecycle) ? readFileSync(lifecycle, "utf8") : "", rgCalls: existsSync(rgCalls) ? readFileSync(rgCalls, "utf8") : "", }; } @@ -292,6 +335,43 @@ describePosix("scripts/pr merge-run", () => { expect(result.calls).not.toContain("--required --watch"); expect(result.calls).not.toContain("--auto"); expect(result.stdout).toContain("merge-run complete for PR #123"); + expect(result.stdout).toContain( + "completion comment: https://github.com/openclaw/openclaw/pull/123#issuecomment-1", + ); + expect(result.commentBody).toBe( + `Merged via squash.\n\n- Prepared head SHA: [${headSha}](https://github.com/openclaw/openclaw/commit/${headSha})\n- Landed commit: [${landedSha}](https://github.com/openclaw/openclaw/commit/${landedSha})`, + ); + expect(result.rgCalls).toBe(""); + expect(result.lifecycle).toBe( + "comment\nremote-cleanup\nworktree-cleanup .worktrees/pr-123\nbranch-cleanup temp/pr-123\nbranch-cleanup pr-123\nbranch-cleanup pr-123-prep\n", + ); + }); + + it("retries transient structured comment failures exactly three times", () => { + const result = runMerge({ commentFailures: 2, mergeStateStatus: "CLEAN" }); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.commentAttempts).toBe(3); + expect(result.lifecycle.match(/^comment$/gmu)).toHaveLength(3); + expect(result.lifecycle).toContain("remote-cleanup"); + }); + + it("fails closed without cleanup when structured comment creation never succeeds", () => { + const result = runMerge({ commentFailures: 3, mergeStateStatus: "CLEAN" }); + + expect(result.status).toBe(1); + expect(result.commentAttempts).toBe(3); + expect(result.stdout).toContain("Failed to post PR comment after retries"); + expect(result.lifecycle).toBe("comment\ncomment\ncomment\n"); + }); + + it("treats an empty structured comment URL as failure and skips cleanup", () => { + const result = runMerge({ commentEmpty: true, mergeStateStatus: "CLEAN" }); + + expect(result.status).toBe(1); + expect(result.commentAttempts).toBe(3); + expect(result.stdout).toContain("Failed to post PR comment after retries"); + expect(result.lifecycle).toBe("comment\ncomment\ncomment\n"); }); it("enables squash auto-merge only for a verified mergeable BEHIND head", () => { diff --git a/test/scripts/pr-wrappers.test.ts b/test/scripts/pr-wrappers.test.ts index ffcfd9a8c71f..ab82fcf8f403 100644 --- a/test/scripts/pr-wrappers.test.ts +++ b/test/scripts/pr-wrappers.test.ts @@ -207,7 +207,8 @@ describe("scripts/pr wrappers", () => { expect(review).toContain('gh_plain pr edit "$pr" --add-assignee "$reviewer"'); expect(push).toContain('gh_plain api graphql --input - <<< "$payload"'); expect(merge).toContain('gh_plain pr merge "$pr"'); - expect(merge).toContain('gh_plain pr comment "$pr"'); + expect(merge).toContain('"repos/{owner}/{repo}/issues/$pr/comments"'); + expect(merge).toContain("--jq '.html_url // empty'"); expect(merge).toContain("gh_plain api -X DELETE"); }); @@ -335,7 +336,7 @@ describe("scripts/pr wrappers", () => { expect(script).toContain("--squash"); expect(script).toContain("--merge"); expect(script).toContain("--rebase"); - expect(script).toContain('echo "Merged via $merge_label."'); + expect(script).toContain("'Merged via %s."); expect(script).toContain("--auto"); expect(script).toContain('--match-head-commit "$PREP_HEAD_SHA"'); });