fix: treat locked-PR 422 as soft skip with retry in safe_outputs (#41155)

Copilot · web-flow · commit b4be41752451 · 2026-06-24T06:59:11.000-07:00
diff --git a/actions/setup/js/pr_review_buffer.cjs b/actions/setup/js/pr_review_buffer.cjs
@@ -24,7 +24,7 @@ const { getErrorMessage } = require("./error_helpers.cjs");
 const { isStagedMode } = require("./safe_output_helpers.cjs");
 const { generateWorkflowCallIdMarker, matchesWorkflowId } = require("./generate_footer.cjs");
 const { attachExecutionState, fetchPullRequestReviewState } = require("./safe_output_execution_metadata.cjs");
-const { withRetry, RATE_LIMIT_RETRY_CONFIG, isTransientError } = require("./error_recovery.cjs");
+const { withRetry, RATE_LIMIT_RETRY_CONFIG, isTransientError, sleep } = require("./error_recovery.cjs");
 
 const SUPERSEDE_REVIEW_MESSAGE = "Superseded by updated review from same workflow.";
 const MAX_SUPERSEDE_REVIEW_PAGES = 10;
@@ -35,6 +35,18 @@ const FALLBACK_EMPTY_COMMENT_BODY = "_(empty comment body)_";
 const FALLBACK_TRUNCATION_SUFFIX = "\n\n_(Fallback review body truncated to fit GitHub length limits.)_";
 const FALLBACK_OMISSION_NOTE = "_(Unanchored comment details omitted to fit GitHub length limits.)_";
 const ELLIPSIS = "…";
+// GitHub API message fragment returned when a PR is locked and review submission is rejected.
+// Must be lowercase — compared against errorMessage.toLowerCase() for case-insensitive matching.
+const LOCKED_PR_REVIEW_MESSAGE = "lock prevents review";
+
+/** Returns true if the error message indicates a locked-PR 422 rejection. */
+const isLockedPrError = errorMessage => errorMessage.toLowerCase().includes(LOCKED_PR_REVIEW_MESSAGE);
+// Number of retries before treating a locked-PR 422 as a permanent soft skip.
+// A small number is used so the run does not stall when the PR is permanently locked.
+const LOCKED_PR_RETRY_COUNT = 3;
+// Delay between lock-retry attempts. Short enough to keep the run responsive
+// while still giving a transient lock a few seconds to clear.
+const LOCKED_PR_RETRY_DELAY_MS = 5000;
 // Keep review retries bounded so safe-outputs can recover from short installation-token
 // quota stalls without spending most of the workflow timeout waiting for a reset.
 const REVIEW_RATE_LIMIT_RETRY_CONFIG = {
@@ -549,13 +561,17 @@ function createReviewBuffer() {
       return beforeState ? fetchReviewStateBestEffort(repoParts, pullRequestNumber, "after") : null;
     }
 
-    try {
-      const { data: review } = await createReviewWithRetry(requestParams);
-      await maybeSupersedeOlderReviews(review.id);
-      const afterState = await fetchAfterStateIfAvailable();
-
-      core.info(`Created PR review #${review.id}: ${review.html_url}`);
-
+    /**
+     * Build the success result payload for a submitted PR review, wrapping it with
+     * execution-state metadata. Extracted to avoid duplicating the shape across the
+     * initial submit, own-PR-COMMENT retry, locked-PR retry, and body-only fallback paths.
+     *
+     * @param {{ id: number, html_url: string, state?: string }} review - Created review object
+     * @param {string} resolvedEvent - The review event actually used (may differ from the requested event)
+     * @param {number} commentCount - Number of inline comments included
+     * @param {import("./safe_output_execution_metadata.cjs").ReviewState|null} afterState - Post-submit review state
+     */
+    function buildReviewSuccessResult(review, resolvedEvent, commentCount, afterState) {
       return attachExecutionState(
         {
           success: true,
@@ -565,17 +581,27 @@ function createReviewBuffer() {
           review_url: review.html_url,
           pull_request_number: pullRequestNumber,
           repo: repo,
-          event: event,
-          comment_count: comments.length,
+          event: resolvedEvent,
+          comment_count: commentCount,
           metadata: {
             review_id: review.id,
-            review_event: event,
+            review_event: resolvedEvent,
             ...(review.state ? { review_state: review.state } : {}),
           },
         },
         beforeState,
         afterState
       );
+    }
+
+    try {
+      const { data: review } = await createReviewWithRetry(requestParams);
+      await maybeSupersedeOlderReviews(review.id);
+      const afterState = await fetchAfterStateIfAvailable();
+
+      core.info(`Created PR review #${review.id}: ${review.html_url}`);
+
+      return buildReviewSuccessResult(review, event, comments.length, afterState);
     } catch (error) {
       const errorMessage = getErrorMessage(error);
 
@@ -591,26 +617,7 @@ function createReviewBuffer() {
           await maybeSupersedeOlderReviews(review.id);
           const afterState = await fetchAfterStateIfAvailable();
           core.info(`Created PR review #${review.id}: ${review.html_url}`);
-          return attachExecutionState(
-            {
-              success: true,
-              url: review.html_url,
-              number: pullRequestNumber,
-              review_id: review.id,
-              review_url: review.html_url,
-              pull_request_number: pullRequestNumber,
-              repo: repo,
-              event: "COMMENT",
-              comment_count: comments.length,
-              metadata: {
-                review_id: review.id,
-                review_event: "COMMENT",
-                ...(review.state ? { review_state: review.state } : {}),
-              },
-            },
-            beforeState,
-            afterState
-          );
+          return buildReviewSuccessResult(review, "COMMENT", comments.length, afterState);
         } catch (retryError) {
           core.error(`Failed to submit PR review on retry: ${getErrorMessage(retryError)}`);
           return {
@@ -620,6 +627,38 @@ function createReviewBuffer() {
         }
       }
 
+      // When the PR is locked, retry a few times to detect if the lock is temporary,
+      // then treat as a soft skip (success:true, skipped:true) so the run is not failed.
+      // GitHub returns 422 with message "lock prevents review" for locked PRs.
+      // We check the error message (which withRetry/enhanceError preserves in "Original error:")
+      // rather than the status code, which may not survive error wrapping.
+      if (isLockedPrError(errorMessage)) {
+        core.warning(`PR #${pullRequestNumber} is locked (422 "${LOCKED_PR_REVIEW_MESSAGE}"). Retrying ${LOCKED_PR_RETRY_COUNT} time(s) to check if the lock is temporary...`);
+        for (let attempt = 1; attempt <= LOCKED_PR_RETRY_COUNT; attempt++) {
+          await sleep(LOCKED_PR_RETRY_DELAY_MS);
+          try {
+            const { data: review } = await createReviewWithRetry(requestParams);
+            await maybeSupersedeOlderReviews(review.id);
+            const afterState = await fetchAfterStateIfAvailable();
+            core.info(`Created PR review #${review.id} after lock retry (attempt ${attempt}/${LOCKED_PR_RETRY_COUNT}): ${review.html_url}`);
+            return buildReviewSuccessResult(review, event, comments.length, afterState);
+          } catch (retryError) {
+            const retryErrorMessage = getErrorMessage(retryError);
+            if (isLockedPrError(retryErrorMessage)) {
+              core.warning(`PR #${pullRequestNumber} is still locked (attempt ${attempt}/${LOCKED_PR_RETRY_COUNT})`);
+            } else {
+              // Different error on retry — surface as a regular failure
+              core.error(`Failed to submit PR review on lock retry attempt ${attempt}: ${retryErrorMessage}`);
+              return { success: false, error: retryErrorMessage };
+            }
+          }
+        }
+        // All retries exhausted — treat as a soft skip so the run stays green
+        const skipMsg = `Review skipped — PR #${pullRequestNumber} is locked`;
+        core.warning(skipMsg);
+        return { success: true, skipped: true, reason: skipMsg, pr_locked: true };
+      }
+
       // When the API cannot resolve a line or path reference in an inline comment, retry as a
       // body-only review so that the overall review (and its footer body) is still submitted
       // successfully. Matches both "Line could not be resolved" and "Path could not be resolved".
@@ -633,26 +672,7 @@ function createReviewBuffer() {
           await maybeSupersedeOlderReviews(review.id);
           const afterState = await fetchAfterStateIfAvailable();
           core.info(`Created PR review #${review.id} (body-only fallback): ${review.html_url}`);
-          return attachExecutionState(
-            {
-              success: true,
-              url: review.html_url,
-              number: pullRequestNumber,
-              review_id: review.id,
-              review_url: review.html_url,
-              pull_request_number: pullRequestNumber,
-              repo: repo,
-              event: event,
-              comment_count: 0,
-              metadata: {
-                review_id: review.id,
-                review_event: event,
-                ...(review.state ? { review_state: review.state } : {}),
-              },
-            },
-            beforeState,
-            afterState
-          );
+          return buildReviewSuccessResult(review, event, 0, afterState);
         } catch (retryError) {
           core.error(`Failed to submit body-only PR review: ${getErrorMessage(retryError)}`);
           return {
diff --git a/actions/setup/js/pr_review_buffer.test.cjs b/actions/setup/js/pr_review_buffer.test.cjs
@@ -855,6 +855,110 @@ describe("pr_review_buffer (factory pattern)", () => {
       expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(2);
     });
 
+    it("should skip (success:true, skipped:true) when PR is permanently locked after all retries", async () => {
+      const setTimeoutSpy = vi.spyOn(global, "setTimeout").mockImplementation(handler => {
+        if (typeof handler === "function") {
+          handler();
+        }
+        return 0;
+      });
+      try {
+        buffer.setReviewMetadata("Looks good", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        const lockedError = Object.assign(new Error("lock prevents review"), { status: 422 });
+        mockGithub.rest.pulls.createReview.mockRejectedValue(lockedError);
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(true);
+        expect(result.skipped).toBe(true);
+        expect(result.pr_locked).toBe(true);
+        expect(result.reason).toContain("locked");
+        // 1 initial + 3 retries
+        expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(4);
+        expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("lock prevents review"));
+        expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Review skipped"));
+      } finally {
+        setTimeoutSpy.mockRestore();
+      }
+    });
+
+    it("should succeed when locked-PR retry eventually unlocks", async () => {
+      const setTimeoutSpy = vi.spyOn(global, "setTimeout").mockImplementation(handler => {
+        if (typeof handler === "function") {
+          handler();
+        }
+        return 0;
+      });
+      try {
+        buffer.setReviewMetadata("Looks good", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        const lockedError = Object.assign(new Error("lock prevents review"), { status: 422 });
+        mockGithub.rest.pulls.createReview
+          .mockRejectedValueOnce(lockedError)
+          .mockRejectedValueOnce(lockedError)
+          .mockResolvedValueOnce({
+            data: {
+              id: 999,
+              html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-999",
+            },
+          });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(true);
+        expect(result.skipped).toBeUndefined();
+        expect(result.review_id).toBe(999);
+        // 1 initial + 2 locked retries (third attempt succeeds)
+        expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(3);
+        expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Retrying 3 time(s)"));
+      } finally {
+        setTimeoutSpy.mockRestore();
+      }
+    });
+
+    it("should return failure when lock retry encounters a different error", async () => {
+      const setTimeoutSpy = vi.spyOn(global, "setTimeout").mockImplementation(handler => {
+        if (typeof handler === "function") {
+          handler();
+        }
+        return 0;
+      });
+      try {
+        buffer.setReviewMetadata("Looks good", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        const lockedError = Object.assign(new Error("lock prevents review"), { status: 422 });
+        const otherError = Object.assign(new Error("internal server error"), { status: 500 });
+        mockGithub.rest.pulls.createReview.mockRejectedValueOnce(lockedError).mockRejectedValueOnce(otherError);
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(false);
+        expect(result.error).toContain("internal server error");
+        expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(2);
+      } finally {
+        setTimeoutSpy.mockRestore();
+      }
+    });
+
     it("should dismiss older reviews matching workflow-call-id when supersede mode is enabled", async () => {
       const previousWorkflowId = process.env.GH_AW_WORKFLOW_ID;
       const previousCallerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID;
diff --git a/actions/setup/js/safe_output_execution_metadata.cjs b/actions/setup/js/safe_output_execution_metadata.cjs
@@ -195,6 +195,10 @@ async function fetchIssueState(github, repoParts, issueNumber) {
   return extractIssueStateFromData(issue);
 }
 
+/**
+ * @typedef {{ requested_reviewers: string[], requested_team_reviewers: string[], reviews: Array<{id?: number, user?: string, state?: string, submitted_at?: string}> }} ReviewState
+ */
+
 function extractReviewStateFromData(pullRequest, reviews) {
   return {
     requested_reviewers: normalizeRequestedReviewers(pullRequest?.requested_reviewers),
diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs
@@ -532,6 +532,28 @@ function rollbackReviewResults(results, errorMessage) {
   }
 }
 
+/**
+ * Mark buffered review results as skipped when the PR is locked and submission was
+ * soft-skipped (success:true, skipped:true). Both submit_pull_request_review and
+ * create_pull_request_review_comment handlers buffer during message processing, so
+ * the skip must be back-propagated here so the Processing Summary reflects the actual
+ * outcome (skipped) rather than a misleading success count.
+ *
+ * Note: uses `skipReason` (not `reason`) so that the step-summary generator does not
+ * treat these entries as delegated-step skips and omit them from the output.
+ *
+ * @param {Array<{type: string, success: boolean, skipped?: boolean, skipReason?: string}>} results - Processing results to mutate
+ * @param {string} skipReason - Human-readable reason for the skip
+ */
+function skipReviewResults(results, skipReason) {
+  for (const r of results) {
+    if ((r.type === "submit_pull_request_review" || r.type === "create_pull_request_review_comment") && r.success === true) {
+      r.skipped = true;
+      r.skipReason = skipReason;
+    }
+  }
+}
+
 /**
  * Determine whether a processing result is a non-skipped, non-deferred, non-cancelled failure.
  *
@@ -1418,6 +1440,13 @@ async function main() {
         if (reviewResult.success && !reviewResult.skipped) {
           logCreatedItemFromResult(logCreatedItem, "submit_pull_request_review", reviewResult);
           core.info(`✓ PR review submitted successfully: ${reviewResult.review_url}`);
+        } else if (reviewResult.success && reviewResult.skipped) {
+          const skipReason = reviewResult.reason || "PR review submission skipped";
+          core.warning(`⚠ ${skipReason}`);
+          if (reviewResult.pr_locked) {
+            core.setOutput("pr_locked", "true");
+          }
+          skipReviewResults(processingResult.results, skipReason);
         } else if (!reviewResult.success) {
           reviewFailureError = reviewResult.error || "PR review finalization failed";
           core.error(`✗ Failed to submit PR review: ${reviewFailureError}`);
@@ -1629,6 +1658,7 @@ module.exports = {
   processMessages,
   buildCommentMemoryMessagesFromFiles,
   rollbackReviewResults,
+  skipReviewResults,
   logCreatedItemFromResult,
   isFailedProcessingResult,
   isReportOnlyFailureResult,
diff --git a/actions/setup/js/safe_output_handler_manager.test.cjs b/actions/setup/js/safe_output_handler_manager.test.cjs
