fix(maintainers): parse merge comment URL structurally (#120649)

2026-08-28 05:16:23 -06:00 · 2026-08-09 01:58:11 +08:00
parent a70deaf14e
commit a07d0e249d
3 changed files with 107 additions and 21 deletions
@@ -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"
@@ -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", () => {
@@ -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"');
  });