Resolve actor account type before applying allowed_bots (#1330)

Move the allowed_bots check in checkHumanActor and checkWritePermissions so
it only fires after the actor has been resolved as a non-User account
(GitHub App / bot, or unresolvable app actor). Actors that resolve to a
regular User account go through the standard human/write checks regardless
of allowed_bots.

The Copilot-style path (GITHUB_ACTOR not ending in [bot] and not resolvable
as a user) is unchanged: it still falls through to the existing 404 catch,
which already consults allowed_bots once the API has reported the actor is
not a user.

Update tests to match and add coverage for the User-account path.

test/actor.test.ts

@@ -97,7 +97,8 @@ describe("checkHumanActor", () => {
   describe("non-[bot] actors (e.g. GitHub Copilot)", () => {
     // GitHub Copilot SWE Agent sets GITHUB_ACTOR="Copilot" which is not a
     // valid GitHub user and doesn't end with [bot], causing 404 on the
-    // Users API. These tests verify the fix handles this gracefully.
+    // Users API. allowed_bots is applied once the API has resolved the
+    // actor as not being a regular user account.
 
     function createMockOctokitThat404s(): Octokit {
       return {
@@ -117,7 +118,6 @@ describe("checkHumanActor", () => {
       context.actor = "Copilot";
       context.inputs.allowedBots = "copilot,cursor";
 
-      // Should not even call the API — allowed_bots check happens first
       await expect(
         checkHumanActor(mockOctokit, context),
       ).resolves.toBeUndefined();
@@ -167,4 +167,52 @@ describe("checkHumanActor", () => {
       ).resolves.toBeUndefined();
     });
   });
+
+  describe("account type resolution", () => {
+    // The Users API resolves the actor's account type before allowed_bots
+    // is consulted. allowed_bots is only relevant for Bot accounts and
+    // unresolvable app actors; it does not change behavior for regular
+    // User accounts.
+
+    test("should pass for a User account whose name matches allowed_bots", async () => {
+      const mockOctokit = createMockOctokit("User");
+      const context = createMockContext();
+      context.actor = "renovate";
+      context.inputs.allowedBots = "renovate";
+
+      await expect(
+        checkHumanActor(mockOctokit, context),
+      ).resolves.toBeUndefined();
+    });
+
+    test("should pass for a User account when allowed_bots is '*'", async () => {
+      const mockOctokit = createMockOctokit("User");
+      const context = createMockContext();
+      context.actor = "some-user";
+      context.inputs.allowedBots = "*";
+
+      await expect(
+        checkHumanActor(mockOctokit, context),
+      ).resolves.toBeUndefined();
+    });
+
+    test("should resolve account type even when actor name appears in allowed_bots", async () => {
+      // The Users API call should not be short-circuited by allowed_bots,
+      // so an unexpected API error propagates instead of being swallowed.
+      const mockOctokit = {
+        users: {
+          getByUsername: async () => {
+            throw new Error("Internal Server Error");
+          },
+        },
+      } as unknown as Octokit;
+      const context = createMockContext();
+      context.actor = "some-user";
+      context.inputs.allowedBots = "some-user";
+
+      await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow(
+        "Internal Server Error",
+      );
+    });
+  });
 });

test/permissions.test.ts

@@ -307,7 +307,9 @@ describe("checkWritePermissions", () => {
   describe("non-[bot] actors (e.g. GitHub Copilot)", () => {
     // GitHub Copilot SWE Agent sets GITHUB_ACTOR="Copilot" which doesn't
     // end with [bot] and is not a valid GitHub user, so the collaborator
-    // permission API returns 404 with "is not a user".
+    // permission API returns 404 with "is not a user". allowed_bots is
+    // applied in that catch path once the API has confirmed the actor is
+    // not a regular user account.
 
     const createMockOctokitThat404s = () =>
       ({
@@ -322,9 +324,7 @@ describe("checkWritePermissions", () => {
         },
       }) as any;
 
-    test("should return true for non-[bot] actor in allowed_bots (pre-API check)", async () => {
-      // The allowed_bots check should happen BEFORE calling the API,
-      // so this should succeed even with a 404-ing mock.
+    test("should return true for non-[bot] app actor in allowed_bots", async () => {
       const mockOctokit = createMockOctokitThat404s();
       const context = createContext();
       context.actor = "Copilot";
@@ -334,11 +334,11 @@ describe("checkWritePermissions", () => {
 
       expect(result).toBe(true);
       expect(coreInfoSpy).toHaveBeenCalledWith(
-        "Actor Copilot is in allowed_bots list, skipping permission check",
+        "Non-user actor Copilot is in allowed_bots list, granting access",
       );
     });
 
-    test("should return true for non-[bot] actor when allowed_bots is '*' (pre-API check)", async () => {
+    test("should return true for non-[bot] app actor when allowed_bots is '*'", async () => {
       const mockOctokit = createMockOctokitThat404s();
       const context = createContext();
       context.actor = "Copilot";
@@ -349,20 +349,18 @@ describe("checkWritePermissions", () => {
       expect(result).toBe(true);
     });
 
-    test("should return true for non-[bot] actor in allowed_bots via 404 fallback", async () => {
-      // Even if somehow we reach the API call (e.g. race condition or
-      // future refactor), the 404 catch path should also check allowed_bots.
+    test("should match config entries written with the [bot] suffix", async () => {
       const mockOctokit = createMockOctokitThat404s();
       const context = createContext();
       context.actor = "SomeNewBot";
-      context.inputs.allowedBots = "somenewbot";
+      context.inputs.allowedBots = "somenewbot[bot]";
 
       const result = await checkWritePermissions(mockOctokit, context);
 
       expect(result).toBe(true);
     });
 
-    test("should return false for non-[bot] actor that 404s and is not in allowed_bots", async () => {
+    test("should return false for non-[bot] app actor that is not in allowed_bots", async () => {
       const mockOctokit = createMockOctokitThat404s();
       const context = createContext();
       context.actor = "Copilot";
@@ -376,7 +374,7 @@ describe("checkWritePermissions", () => {
       );
     });
 
-    test("should return false for non-[bot] actor that 404s with empty allowed_bots", async () => {
+    test("should return false for non-[bot] app actor with empty allowed_bots", async () => {
       const mockOctokit = createMockOctokitThat404s();
       const context = createContext();
       context.actor = "Copilot";
@@ -404,4 +402,57 @@ describe("checkWritePermissions", () => {
       );
     });
   });
+
+  describe("allowed_bots only applies to non-user actors", () => {
+    // The permission endpoint resolves the actor's account type. Actors
+    // that resolve to a regular user account go through the standard write
+    // permission check; allowed_bots does not short-circuit it for them.
+
+    test("should require write permission for a user account whose name matches allowed_bots", async () => {
+      const mockOctokit = createMockOctokit("read");
+      const context = createContext();
+      context.actor = "renovate";
+      context.inputs.allowedBots = "renovate";
+
+      const result = await checkWritePermissions(mockOctokit, context);
+
+      expect(result).toBe(false);
+      expect(coreWarningSpy).toHaveBeenCalledWith(
+        "Actor has insufficient permissions: read",
+      );
+    });
+
+    test("should require write permission for a user account when allowed_bots uses the [bot] form", async () => {
+      const mockOctokit = createMockOctokit("read");
+      const context = createContext();
+      context.actor = "renovate";
+      context.inputs.allowedBots = "renovate[bot]";
+
+      const result = await checkWritePermissions(mockOctokit, context);
+
+      expect(result).toBe(false);
+    });
+
+    test("should require write permission for a user account when allowed_bots is '*'", async () => {
+      const mockOctokit = createMockOctokit("none");
+      const context = createContext();
+      context.actor = "some-user";
+      context.inputs.allowedBots = "*";
+
+      const result = await checkWritePermissions(mockOctokit, context);
+
+      expect(result).toBe(false);
+    });
+
+    test("should still grant access for a user account with write permission", async () => {
+      const mockOctokit = createMockOctokit("write");
+      const context = createContext();
+      context.actor = "renovate";
+      context.inputs.allowedBots = "renovate";
+
+      const result = await checkWritePermissions(mockOctokit, context);
+
+      expect(result).toBe(true);
+    });
+  });
 });