fix(safe-outputs): treat stale/resolved review threads as no-ops; allow bot review dismissal (#49648)

Author: Copilot

.github/workflows/pr-sous-chef.lock.yml

@@ -377,6 +377,7 @@ For each PR that is not skipped:
    - For `schedule` and `workflow_dispatch` runs, use the `resolve_review_threads` list returned by the `pr-processor` sub-agent.
    - Include a thread only when all of the following are true: the thread is currently unresolved; contains reviewer feedback; and has a later reply from the PR author or `@copilot`.
    - For each thread ID, call `safeoutputs resolve_pull_request_review_thread --thread_id <ID>`.
+   - Each `<ID>` must be the review thread node ID (`PRRT_...`) taken from `reviewThreads`; never pass a review comment node ID (`PRRC_...`).
    - If resolving one thread fails, record `{thread_id: <ID>, skip_reason: "resolve_review_thread_failed"}` in the `skipped` array and continue.
 
 4. **Dismiss stale `github-actions[bot]` blocking reviews when all PR review threads are resolved**
@@ -446,7 +447,7 @@ Given one PR number and compact metadata:
    - a single combined nudge comment body:
      - if `conflicting` is true: a targeted nudge asking `@copilot` to run `make merge-main` to resolve conflicts
      - otherwise: a combined nudge covering unresolved review feedback, failed checks (from `failed_checks` in the compact JSON — list each by name with URL when available), branch refresh, and any other forward-progress action including a direct instruction to run the `pr-finisher` skill — one comment only, never two; if unresolved PR reviews exist, include an explicit unresolved-reviews list (reviewer + direct link per unresolved review thread)
-   - `resolve_review_threads`: an array of unresolved PR review thread node IDs to resolve via safe output; include a thread only when the thread already contains a follow-up response from the PR author or `@copilot` that addresses the feedback
+   - `resolve_review_threads`: an array of unresolved PR review thread node IDs (`PRRT_...`) from `reviewThreads` to resolve via safe output; never emit review comment node IDs (`PRRC_...`). Include a thread only when the thread already contains a follow-up response from the PR author or `@copilot` that addresses the feedback
    - `dismiss_reviews`: an array of review IDs — include a review ID only when the review was authored by `github-actions[bot]` with `CHANGES_REQUESTED` state AND all review threads on the PR are resolved (no unresolved threads remain); return an empty array if there are unresolved threads or no qualifying reviews
 4. Make at most 8 tool calls total. If 8 calls are insufficient to reach a confident decision, set all fields to `null` and set `skip_reason: "insufficient_context"`.
 5. Keep output compact JSON only — a single object, no prose.

.github/workflows/pr-sous-chef.md

@@ -233,6 +233,7 @@ async function main(config = {}) {
       });
 
       const reviewAuthorLogin = review?.user?.login;
+      const reviewAuthorType = typeof review?.user?.type === "string" ? review.user.type.trim() : "";
       if (typeof reviewAuthorLogin !== "string" || reviewAuthorLogin.trim() === "") {
         return {
           success: false,
@@ -241,6 +242,18 @@ async function main(config = {}) {
       }
       const reviewAuthor = reviewAuthorLogin.trim();
       if (reviewAuthor !== expectedAuthor) {
+        if (reviewAuthorType === "Bot") {
+          const warningMessage =
+            `Skipping dismiss_pull_request_review for review ${reviewId}: ` +
+            `review author (${reviewAuthor}) does not match dismisser (${dismisser}). ` +
+            `Actor-bound dismissal only permits dismissing reviews authored by the current workflow actor.`;
+          core.warning(warningMessage);
+          return {
+            success: false,
+            skipped: true,
+            error: warningMessage,
+          };
+        }
         return {
           success: false,
           error: `review author (${reviewAuthor || "unknown"}) must match dismisser (${dismisser})`,

.github/workflows/smoke-checkout-pr-dispatch.lock.yml

@@ -50,7 +50,7 @@ describe("dismiss_pull_request_review", () => {
     mockGetReview.mockResolvedValue({
       data: {
         html_url: "https://github.com/test-owner/test-repo/pull/42#pullrequestreview-123",
-        user: { login: "github-actions[bot]" },
+        user: { login: "github-actions[bot]", type: "Bot" },
       },
     });
     mockDismissReview.mockResolvedValue({
@@ -118,7 +118,7 @@ describe("dismiss_pull_request_review", () => {
   it("rejects when fetched review author differs from current actor", async () => {
     mockGetReview.mockResolvedValueOnce({
       data: {
-        user: { login: "octocat" },
+        user: { login: "octocat", type: "User" },
       },
     });
 
@@ -133,6 +133,30 @@ describe("dismiss_pull_request_review", () => {
     expect(mockDismissReview).not.toHaveBeenCalled();
   });
 
+  it("skips dismissal when review was authored by a bot and actor is a different user", async () => {
+    process.env.GITHUB_ACTOR = "pelikhan";
+    const { main } = require("./dismiss_pull_request_review.cjs");
+    handler = await main({ max: 10 });
+
+    mockGetReview.mockResolvedValueOnce({
+      data: {
+        html_url: "https://github.com/test-owner/test-repo/pull/42#pullrequestreview-123",
+        user: { login: "github-actions[bot]", type: "Bot" },
+      },
+    });
+
+    const result = await handler({
+      type: "dismiss_pull_request_review",
+      review_id: 123,
+      justification: "Dismissing stale github-actions review because all PR review threads are resolved.",
+    });
+
+    expect(result.success).toBe(false);
+    expect(result.skipped).toBe(true);
+    expect(result.error).toContain("Actor-bound dismissal only permits");
+    expect(mockDismissReview).not.toHaveBeenCalled();
+  });
+
   it("resolves review_id=auto to all dismissible reviews by current actor", async () => {
     mockListReviews.mockResolvedValueOnce({
       data: [

actions/setup/js/dismiss_pull_request_review.cjs

@@ -22,13 +22,19 @@ const HANDLER_TYPE = "resolve_pull_request_review_thread";
  * Used to validate the thread before resolving.
  * @param {any} github - GitHub GraphQL instance
  * @param {string} threadId - Review thread node ID (e.g., 'PRRT_kwDOABCD...')
- * @returns {Promise<{prNumber: number, repoNameWithOwner: string|null}|null>} The PR number and repo, or null if not found
+ * @returns {Promise<
+ *   | {status: "missing"}
+ *   | {status: "thread", prNumber: number, repoNameWithOwner: string|null, isResolved: boolean}
+ *   | {status: "invalid_type", nodeType: string}
+ * >} Thread lookup result
  */
 async function getThreadPullRequestInfo(github, threadId) {
   const query = /* GraphQL */ `
     query ($threadId: ID!) {
       node(id: $threadId) {
+        __typename
         ... on PullRequestReviewThread {
+          isResolved
           pullRequest {
             number
             repository {
@@ -42,14 +48,23 @@ async function getThreadPullRequestInfo(github, threadId) {
 
   const result = await github.graphql(query, { threadId });
 
-  const pullRequest = result?.node?.pullRequest;
-  if (!pullRequest) {
-    return null;
+  const threadNode = result?.node;
+  if (!threadNode) {
+    return { status: "missing" };
+  }
+  if (threadNode.__typename !== "PullRequestReviewThread") {
+    return {
+      status: "invalid_type",
+      nodeType: threadNode.__typename || "unknown",
+    };
   }
+  const pullRequest = threadNode.pullRequest;
 
   return {
+    status: "thread",
     prNumber: pullRequest.number,
     repoNameWithOwner: pullRequest.repository?.nameWithOwner ?? null,
+    isResolved: threadNode?.isResolved === true,
   };
 }
 
@@ -174,15 +189,24 @@ async function main(config = {}) {
 
       // Look up the thread's PR number and repository
       const threadInfo = await getThreadPullRequestInfo(githubClient, threadId);
-      if (threadInfo === null) {
-        core.warning(`Review thread not found or not a PullRequestReviewThread: ${threadId}`);
+      if (threadInfo.status === "missing") {
+        core.info(`Review thread ${threadId} not found — already resolved or stale; skipping`);
+        return {
+          success: true,
+          thread_id: threadId,
+          is_resolved: true,
+          skipped: true,
+        };
+      }
+
+      if (threadInfo.status !== "thread") {
         return {
           success: false,
-          error: `Review thread not found: ${threadId}`,
+          error: `thread_id must reference a PullRequestReviewThread node ID (PRRT_...); received ${threadInfo.nodeType} for ${threadId}`,
         };
       }
 
-      const { prNumber: threadPRNumber, repoNameWithOwner: threadRepo } = threadInfo;
+      const { prNumber: threadPRNumber, repoNameWithOwner: threadRepo, isResolved } = threadInfo;
 
       // When the user explicitly configured target-repo or allowed-repos, validate the thread's
       // repository using validateTargetRepo (supports wildcards like "*", "org/*").
@@ -282,6 +306,16 @@ async function main(config = {}) {
       const filterResult = await checkRequiredFilter(githubClient, repoParts, threadPRNumber, requiredLabels, requiredTitlePrefix, "resolve_pull_request_review_thread");
       if (filterResult) return filterResult;
 
+      if (isResolved) {
+        core.info(`Review thread ${threadId} is already resolved; skipping`);
+        return {
+          success: true,
+          thread_id: threadId,
+          is_resolved: true,
+          skipped: true,
+        };
+      }
+
       // If in staged mode, preview without executing
       if (isStaged) {
         logStagedPreviewInfo(`Would resolve review thread ${threadId}`);