fix(gh-design): skip self-review requests and tolerate 422 errors by snomiao · Pull Request #202 · Comfy-Org/Comfy-PR

snomiao · 2026-04-22T13:06:12Z

Summary

The 5-minute Combined GitHub Tasks workflow has been failing every run. Root cause:

GitHub Design Task: 21128ms - Review cannot be requested from pull request author.
1 task(s) failed. Exiting with error code 1.

REQUEST_REVIEWERS is hardcoded to ["PabloWiedemann"] in app/tasks/gh-design/gh-design.ts. When Pablo opens a PR himself, the task calls gh.pulls.requestReviewers with him as the reviewer, GitHub responds 422 Review cannot be requested from pull request author, and run-gh-tasks.ts then exits non-zero — turning one avoidable PR-level error into a full workflow failure on every 5-minute tick.

Fix

Skip self-review: filter the PR author (task.user) out of the reviewer list before calling the API (case-insensitive comparison).
Tolerate 422: wrap the call in a try/catch that swallows only status === 422 (non-collaborators, etc.), rethrowing anything else. 422s are retried on subsequent runs since the condition may be transient.
Persist only on success: reviewer list is saved to DB only after a successful API call, keeping DB state accurate.
No redundant retries: the guard condition uses the filtered newReviewers list, so when all eligible reviewers are already requested, the block is skipped entirely.
Extracted filterReviewers() into a side-effect-free module with 8 unit tests covering author filtering, case insensitivity, and edge cases.

Test plan

CI run_comfy_pr and run_combined_github_tasks pass
filterReviewers.spec.ts — 8 tests passing
Next scheduled Combined GitHub Tasks run finishes green.
PRs by non-authors still get reviewer requests as before.
PRs authored by a REQUEST_REVIEWERS member skip the call silently.

The 5-minute combined GitHub tasks workflow has been failing every run because GitHub Design Task tried to request review from PabloWiedemann on PRs he authored himself. GitHub rejects this with: 422 Review cannot be requested from pull request author. Since run-gh-tasks.ts exits non-zero on any sub-task failure, this one error takes the whole scheduled job down on every run. Fix: - Filter the PR author out of the reviewers list before calling requestReviewers, so self-authored PRs don't trigger the 422. - Catch 422 responses defensively so an unexpected rejection (e.g. a reviewer who has left the org, a collaborator permission change) does not fail the task. - Persist the reviewers list even when no API call is needed, so the task doesn't re-attempt the same request on every 5-minute tick. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

vercel · 2026-04-22T13:06:14Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
comfy-pr	Ready	Preview, Comment	Apr 30, 2026 11:15am

Copilot

Pull request overview

Fixes the scheduled “GitHub Design Task” workflow failure by preventing invalid reviewer-request API calls (self-review) and handling expected 422 responses without failing the whole run.

Changes:

Filters the PR author out of the reviewer request list to avoid self-review requests.
Wraps gh.pulls.requestReviewers with 422-tolerant error handling.
Persists the attempted reviewer set to avoid retrying every scheduled run.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-22T13:11:22Z

+            if (newReviewers.length > 0) {
+              try {
+                await gh.pulls.requestReviewers({
+                  owner,
+                  repo,
+                  pull_number: issue_number,
+                  reviewers: newReviewers,
+                });
+              } catch (err: any) {
+                // GitHub returns 422 when the requested reviewer is the PR author
+                // or otherwise cannot be added. Record the attempt to avoid
+                // retrying on every 5-minute schedule run.
+                if (err?.status !== 422) throw err;
+                tlog(`Reviewer request rejected (422): ${err.message}`);
+              }


This change adds new branching behavior (self-review filtering, skipping API call when empty, swallowing only 422s) but there’s no automated coverage around the reviewer-request flow. Consider adding a small bun:test spec that exercises self-authored PRs (no API call, reviewers persisted) and a simulated 422 from gh.pulls.requestReviewers (task continues and persists the attempt) to prevent regressions.

Added in f97f018 — extracted filterReviewers() as a pure function and added filterReviewers.spec.ts with 6 tests covering: self-authored PRs (empty list), already-requested reviewers, mixed scenarios, and undefined alreadyRequested.

Copilot · 2026-04-22T13:11:22Z

          const requestReviewers = REQUEST_REVIEWERS;
-          const newReviewers = requestReviewers.filter((e) => !task.reviewers?.includes(e));
-          tlog(`Requesting reviewers: ${newReviewers.join(", ")}`);
+          const newReviewers = requestReviewers.filter(
+            (e) => !task.reviewers?.includes(e) && e !== task.user,
+          );
+          tlog(`Requesting reviewers: ${newReviewers.join(", ") || "(none)"}`);


task.reviewers is documented/used as the set of requested reviewers, but this code persists requestReviewers = REQUEST_REVIEWERS (unfiltered) even when the PR author was filtered out. That will make the dashboard show the author as a requested reviewer even though the API call was skipped. Consider defining requestReviewers as the filtered base list (e.g., excluding task.user) and updating the guard condition to use that same list, so you can persist accurate reviewer data while still avoiding retries when the filtered list is empty.

Fixed in f97f018 — requestReviewers now filters out task.user before use, so persisted data accurately reflects only the reviewers who were actually eligible to be requested.

Copilot · 2026-04-22T13:11:23Z

+              } catch (err: any) {
+                // GitHub returns 422 when the requested reviewer is the PR author
+                // or otherwise cannot be added. Record the attempt to avoid
+                // retrying on every 5-minute schedule run.
+                if (err?.status !== 422) throw err;
+                tlog(`Reviewer request rejected (422): ${err.message}`);


Catching err: any makes it easy to accidentally swallow/handle unexpected error shapes. Prefer unknown and narrow to Octokit’s request error type (or a small structural check) before reading status/message, so the 422 handling is type-safe and resilient to non-Octokit exceptions.

Fixed in f97f018 — changed catch (err: any) to catch (err: unknown) with structural narrowing via (err as { status?: number })?.status.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…Org/Comfy-PR into fix/gh-design-self-review

The 5-minute combined GitHub tasks workflow has been failing every run because GitHub Design Task tried to request review from PabloWiedemann on PRs he authored himself. GitHub rejects this with: 422 Review cannot be requested from pull request author. Since run-gh-tasks.ts exits non-zero on any sub-task failure, this one error takes the whole scheduled job down on every run. Fix: - Filter the PR author out of the reviewers list before calling requestReviewers, so self-authored PRs don't trigger the 422. - Catch 422 responses defensively so an unexpected rejection (e.g. a reviewer who has left the org, a collaborator permission change) does not fail the task. - Persist the reviewers list even when no API call is needed, so the task doesn't re-attempt the same request on every 5-minute tick. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…from persisted reviewers - Filter task.user from requestReviewers (not just newReviewers) so persisted data accurately reflects who was actually requested - Use catch (err: unknown) with structural narrowing instead of any for type-safe 422 handling Amp-Thread-ID: https://ampcode.com/threads/T-019ddde1-db08-71ca-809d-fd7e51b68842 Co-authored-by: Amp <amp@ampcode.com>

Extract reviewer filtering into a testable pure function and add tests covering: self-authored PRs, already-requested reviewers, empty reviewer lists, and undefined alreadyRequested. Amp-Thread-ID: https://ampcode.com/threads/T-019ddde1-db08-71ca-809d-fd7e51b68842 Co-authored-by: Amp <amp@ampcode.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-30T10:25:08Z

+import { describe, expect, it } from "bun:test";
+import { filterReviewers } from "./gh-design";
+
+describe("filterReviewers", () => {
+  const REVIEWERS = ["PabloWiedemann", "AliceDev"];
+


This test imports filterReviewers from ./gh-design, but gh-design.ts imports @/src/db which performs a top-level MongoClient initialization (top-level await) and will try to connect during tests unless @/src/db is mocked. This can make the unit test suite hang/fail when MONGODB_URI isn’t set. Consider either moving filterReviewers into a small side-effect-free module (e.g. filterReviewers.ts) that both the task and the test import, or change the spec to mock.module("@/src/db", ...) before dynamically importing ./gh-design.

Suggested change

import { describe, expect, it } from "bun:test";

import { filterReviewers } from "./gh-design";

describe("filterReviewers", () => {

const REVIEWERS = ["PabloWiedemann", "AliceDev"];

import { beforeAll, describe, expect, it, mock } from "bun:test";

let filterReviewers: typeof import("./gh-design")["filterReviewers"];

describe("filterReviewers", () => {

const REVIEWERS = ["PabloWiedemann", "AliceDev"];

beforeAll(async () => {

mock.module("@/src/db", () => ({

db: {},

}));

({ filterReviewers } = await import("./gh-design"));

});

Fixed in cee9996 — extracted filterReviewers() into its own side-effect-free module (filterReviewers.ts). Test now imports directly from it without triggering any DB initialization. Test time dropped from ~1126ms to ~148ms.

Copilot · 2026-04-30T10:25:08Z

@@ -258,16 +272,31 @@ export async function runGithubDesignTask() {
          task.type === "pull_request" &&
          REQUEST_REVIEWERS.some((e) => !task.reviewers?.includes(e))
        ) {
-          const requestReviewers = REQUEST_REVIEWERS;
-          const newReviewers = requestReviewers.filter((e) => !task.reviewers?.includes(e));
-          tlog(`Requesting reviewers: ${newReviewers.join(", ")}`);
+          const { requestReviewers, newReviewers } = filterReviewers(
+            REQUEST_REVIEWERS,
+            task.user,
+            task.reviewers,
+          );


The guard condition uses REQUEST_REVIEWERS.some((e) => !task.reviewers?.includes(e)), but task.reviewers is persisted as the filtered requestReviewers (excluding task.user). If the PR author is in REQUEST_REVIEWERS, task.reviewers will never include them, so this condition remains true forever and the task will re-enter this block (and re-save) on every 5‑minute run. Update the condition to compare against the same filtered/eligible reviewer set you persist (e.g., compute eligible reviewers excluding task.user first, and use that list both for the some(...) check and for saving).

Fixed in cee9996 — removed the REQUEST_REVIEWERS.some(...) guard and replaced it with newReviewers.length > 0 check after computing the filtered list. Now when the PR author is in REQUEST_REVIEWERS and has already been filtered out, the block won't re-enter and re-save on every tick.

…le, fix guard condition - Move filterReviewers() into its own module to avoid DB side effects in tests (Copilot review comment) - Fix guard condition: use filtered newReviewers.length instead of comparing against unfiltered REQUEST_REVIEWERS, which caused the task to re-enter and re-save on every 5-minute tick when the author was in the reviewer list (Copilot review comment) Amp-Thread-ID: https://ampcode.com/threads/T-019ddde1-db08-71ca-809d-fd7e51b68842 Co-authored-by: Amp <amp@ampcode.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-30T10:33:39Z

+                const status = (err as { status?: number })?.status;
+                if (status !== 422) throw err;
+                tlog(`Reviewer request rejected (422): ${err}`);
+              }
+              task = await saveGithubDesignTask(url, { reviewers: requestReviewers });


On a 422, this code persists reviewers: requestReviewers even though GitHub rejected the request. Since reviewers is later displayed in the dashboard as “requested reviewers”, this will show reviewers as requested when they weren’t. Consider persisting an explicit flag/error (e.g., reviewerRequestRejected: true / reviewerRequestError) or storing attempted reviewers in a separate field, so the DB/UI state remains accurate while still preventing retries.

Fixed — reviewers are now only persisted to DB on successful API call. On 422 rejection, the task will retry on the next run.

Copilot · 2026-04-30T10:33:39Z

+export function filterReviewers(
+  allReviewers: string[],
+  prAuthor: string,
+  alreadyRequested?: string[],
+): { requestReviewers: string[]; newReviewers: string[] } {
+  const requestReviewers = allReviewers.filter((e) => e !== prAuthor);
+  const newReviewers = requestReviewers.filter((e) => !alreadyRequested?.includes(e));


filterReviewers compares GitHub usernames case-sensitively (e !== prAuthor / includes). Since GitHub logins are effectively case-insensitive and reviewer lists can be edited via the task config UI, normalizing to a consistent case for comparisons (while returning original casing for API/display) would make the behavior more robust.

Suggested change

export function filterReviewers(

allReviewers: string[],

prAuthor: string,

alreadyRequested?: string[],

): { requestReviewers: string[]; newReviewers: string[] } {

const requestReviewers = allReviewers.filter((e) => e !== prAuthor);

const newReviewers = requestReviewers.filter((e) => !alreadyRequested?.includes(e));

function normalizeGitHubLogin(login: string): string {

return login.toLowerCase();

}

export function filterReviewers(

allReviewers: string[],

prAuthor: string,

alreadyRequested?: string[],

): { requestReviewers: string[]; newReviewers: string[] } {

const normalizedPrAuthor = normalizeGitHubLogin(prAuthor);

const normalizedAlreadyRequested = new Set(

alreadyRequested?.map((reviewer) => normalizeGitHubLogin(reviewer)) ?? [],

);

const requestReviewers = allReviewers.filter(

(e) => normalizeGitHubLogin(e) !== normalizedPrAuthor,

);

const newReviewers = requestReviewers.filter(

(e) => !normalizedAlreadyRequested.has(normalizeGitHubLogin(e)),

);

Fixed in e43396f — filterReviewers() now normalizes usernames to lowercase for all comparisons while preserving original casing in the returned arrays. Added 2 tests for case-insensitive matching.

On 422 rejection, don't save the reviewer list to the database since the reviewers were never actually requested. This keeps DB state accurate and allows retry on subsequent runs. Amp-Thread-ID: https://ampcode.com/threads/T-019ddde1-db08-71ca-809d-fd7e51b68842 Co-authored-by: Amp <amp@ampcode.com>

GitHub logins are case-insensitive, so normalize comparisons in filterReviewers() to prevent edge cases with mixed-case usernames. Added 2 tests for case-insensitive matching. Amp-Thread-ID: https://ampcode.com/threads/T-019ddde1-db08-71ca-809d-fd7e51b68842 Co-authored-by: Amp <amp@ampcode.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-30T10:41:44Z

+                // GitHub may return 422 when a requested reviewer cannot be added,
+                // such as when they are not a collaborator, cannot be requested,
+                // or have already been requested. Record the attempt to avoid
+                // retrying on every 5-minute schedule run.
+                const status = (err as { status?: number })?.status;
+                if (status !== 422) throw err;
+                tlog(`Reviewer request rejected (422): ${err}`);
+              }


The 422 handler comment says the attempt is recorded to avoid retrying on every 5‑minute run, but the code does not persist anything on 422 (and also doesn’t persist when newReviewers is empty). As a result, a persistent 422 (e.g., non-collaborator) will be retried every run, and the comment/PR description are misleading. Either (a) update the comment/PR description to reflect that 422s will be retried, or (b) persist a separate “attempted/rejected reviewers” state (or an error flag) so retries are actually suppressed without marking reviewers as successfully requested.

Fixed in 29caf1f — updated the comment to accurately state that 422 rejections are retried on subsequent runs (not suppressed), since the reviewer may become eligible later. This is intentional: the primary self-review case is already filtered out, and remaining 422s (e.g., non-collaborator) are transient conditions worth retrying.

Update comment to accurately state that 422 rejections are retried on subsequent runs (not suppressed), since the reviewer may become eligible later. Amp-Thread-ID: https://ampcode.com/threads/T-019ddde1-db08-71ca-809d-fd7e51b68842 Co-authored-by: Amp <amp@ampcode.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-30T11:11:35Z

+ * Filter the reviewer list for a PR: excludes the PR author and
+ * anyone who has already been requested.


The module docstring says the filter excludes both the PR author and “anyone who has already been requested”, but requestReviewers only filters out the author (already-requested reviewers are only excluded from newReviewers). This comment is currently misleading—update it to describe the two return lists accurately (e.g., requestReviewers = eligible reviewers excluding author; newReviewers = eligible reviewers not yet requested).

Suggested change

* Filter the reviewer list for a PR: excludes the PR author and

* anyone who has already been requested.

* Filter the reviewer list for a PR.

* `requestReviewers` contains eligible reviewers excluding the PR author.

* `newReviewers` contains eligible reviewers who have not already been requested.

Fixed in add499b — updated the docstring to accurately describe the two return values: requestReviewers (minus author) and newReviewers (minus author and already-requested).

Copilot · 2026-04-30T11:11:36Z

+        if (task.type === "pull_request") {
+          const { requestReviewers, newReviewers } = filterReviewers(
+            REQUEST_REVIEWERS,
+            task.user,
+            task.reviewers,
+          );
+          if (newReviewers.length > 0) {
+            tlog(`Requesting reviewers: ${newReviewers.join(", ")}`);
+            if (!dryRun) {
+              let reviewersRequested = false;
+              try {
+                await gh.pulls.requestReviewers({
+                  owner,
+                  repo,
+                  pull_number: issue_number,
+                  reviewers: newReviewers,
+                });
+                reviewersRequested = true;
+              } catch (err: unknown) {
+                // GitHub may return 422 when a requested reviewer cannot be added,
+                // such as when they are not a collaborator or cannot be requested.
+                // We log but don't persist, so the request will be retried on the
+                // next run (the reviewer may become eligible later).
+                const status = (err as { status?: number })?.status;
+                if (status !== 422) throw err;
+                tlog(`Reviewer request rejected (422): ${err}`);
+              }
+              if (reviewersRequested) {
+                task = await saveGithubDesignTask(url, { reviewers: requestReviewers });
+              }


PR description says the task will persist the attempted reviewers list even when the filtered list is empty to avoid retrying on every schedule tick, but this implementation only persists reviewers after a successful requestReviewers call and does nothing when newReviewers.length === 0. Either update the PR description to match the behavior, or persist some state when newReviewers is empty (without marking reviewers as successfully requested) if suppressing future work is still desired.

Fixed — updated PR description to accurately describe the persistence behavior: reviewers are only saved on successful API call, and 422s are retried on subsequent runs.

Clarify that requestReviewers excludes only the author while newReviewers additionally excludes already-requested reviewers. Amp-Thread-ID: https://ampcode.com/threads/T-019ddde1-db08-71ca-809d-fd7e51b68842 Co-authored-by: Amp <amp@ampcode.com>

Copilot AI review requested due to automatic review settings April 22, 2026 13:06

Copilot started reviewing on behalf of snomiao April 22, 2026 13:06 View session

vercel Bot deployed to Preview April 22, 2026 13:07 View deployment

Copilot AI reviewed Apr 22, 2026

View reviewed changes

snomiao enabled auto-merge (squash) April 24, 2026 00:17

Update app/tasks/gh-design/gh-design.ts

4cf8723

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

vercel Bot deployed to Preview April 24, 2026 00:19 View deployment

snomiao and others added 5 commits April 29, 2026 04:36

Merge branch 'fix/gh-design-self-review' of https://github.com/Comfy-…

2c8cfd9

…Org/Comfy-PR into fix/gh-design-self-review

Update app/tasks/gh-design/gh-design.ts

7472a00

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

snomiao force-pushed the fix/gh-design-self-review branch from 4cf8723 to f97f018 Compare April 30, 2026 10:19

vercel Bot deployed to Preview April 30, 2026 10:21 View deployment

snomiao requested a review from Copilot April 30, 2026 10:22

Copilot started reviewing on behalf of snomiao April 30, 2026 10:22 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

vercel Bot deployed to Preview April 30, 2026 10:28 View deployment

snomiao requested a review from Copilot April 30, 2026 10:29

fix(gh-design): resolve merge conflict — adopt filterReviewers helper

7ab8d77

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

vercel Bot deployed to Preview April 30, 2026 10:32 View deployment

Copilot AI reviewed Apr 30, 2026

View reviewed changes

snomiao and others added 2 commits April 30, 2026 10:35

vercel Bot deployed to Preview April 30, 2026 10:36 View deployment

snomiao requested a review from Copilot April 30, 2026 10:38

Copilot started reviewing on behalf of snomiao April 30, 2026 10:38 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

vercel Bot deployed to Preview April 30, 2026 10:44 View deployment

snomiao requested a review from Copilot April 30, 2026 11:08

Copilot started reviewing on behalf of snomiao April 30, 2026 11:09 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

vercel Bot deployed to Preview April 30, 2026 11:15 View deployment

		* Filter the reviewer list for a PR: excludes the PR author and
		* anyone who has already been requested.

- * Filter the reviewer list for a PR: excludes the PR author and
- * anyone who has already been requested.
+ * Filter the reviewer list for a PR.
+ * `requestReviewers` contains eligible reviewers excluding the PR author.
+ * `newReviewers` contains eligible reviewers who have not already been requested.

Conversation

snomiao commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Test plan

Uh oh!

vercel Bot commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

snomiao commented Apr 22, 2026 •

edited

Loading

vercel Bot commented Apr 22, 2026 •

edited

Loading