diff --git a/src/github/api/queries/github.ts b/src/github/api/queries/github.ts index a6e8a185..eacae45d 100644 --- a/src/github/api/queries/github.ts +++ b/src/github/api/queries/github.ts @@ -7,6 +7,7 @@ export const PR_QUERY = ` title body author { + __typename login } baseRefName @@ -57,6 +58,7 @@ export const PR_QUERY = ` databaseId body author { + __typename login } createdAt @@ -70,6 +72,7 @@ export const PR_QUERY = ` id databaseId author { + __typename login } body @@ -86,6 +89,7 @@ export const PR_QUERY = ` line diffHunk author { + __typename login } createdAt @@ -108,6 +112,7 @@ export const ISSUE_QUERY = ` title body author { + __typename login } createdAt @@ -125,6 +130,7 @@ export const ISSUE_QUERY = ` databaseId body author { + __typename login } createdAt diff --git a/src/github/data/fetcher.ts b/src/github/data/fetcher.ts index fb492ea4..0de99782 100644 --- a/src/github/data/fetcher.ts +++ b/src/github/data/fetcher.ts @@ -23,6 +23,7 @@ import type { CommentWithImages } from "../utils/image-downloader"; import { downloadCommentImages } from "../utils/image-downloader"; import { parseActorFilter, + resolveActorName, shouldIncludeCommentByActor, } from "../utils/actor-filter"; @@ -339,7 +340,7 @@ export function isBodySafeToUse( * @returns Filtered array of comments */ export function filterCommentsByActor< - T extends { author: { login: string } | null }, + T extends { author: { login: string; __typename?: string } | null }, >(comments: T[], includeActors: string = "", excludeActors: string = ""): T[] { const includeParsed = parseActorFilter(includeActors); const excludeParsed = parseActorFilter(excludeActors); @@ -351,9 +352,10 @@ export function filterCommentsByActor< return comments.filter((comment) => shouldIncludeCommentByActor( - // author is null for comments from deleted ("ghost") accounts; treat them - // as the "ghost" login so filtering never dereferences null and crashes. - comment.author?.login ?? "ghost", + // Normalizes App actors to their "[bot]"-suffixed name, which is the form + // filter patterns are written in. Also maps deleted ("ghost") accounts, + // whose author is null, to "ghost" so filtering never dereferences null. + resolveActorName(comment.author), includeParsed, excludeParsed, ), diff --git a/src/github/types.ts b/src/github/types.ts index c5140c12..280073c6 100644 --- a/src/github/types.ts +++ b/src/github/types.ts @@ -3,9 +3,14 @@ // GitHub's GraphQL `author`/`actor` fields resolve to null when the underlying // account has been deleted (the "ghost" user). Any field typed as // `GitHubAuthor | null` can therefore be null at runtime and must be guarded. +// `__typename` distinguishes an App/bot actor from a human. GraphQL's +// `Actor.login` returns the bare name for bots ("dependabot"), unlike REST which +// appends a suffix ("dependabot[bot]"), so the typename is the only reliable bot +// signal on this data. See `resolveActorName` in `utils/actor-filter.ts`. export type GitHubAuthor = { login: string; name?: string; + __typename?: string; }; export type GitHubComment = { diff --git a/src/github/utils/actor-filter.ts b/src/github/utils/actor-filter.ts index 2aebae70..91db5dc9 100644 --- a/src/github/utils/actor-filter.ts +++ b/src/github/utils/actor-filter.ts @@ -11,6 +11,31 @@ export function parseActorFilter(filterString: string): string[] { .filter((actor) => actor.length > 0); } +/** + * Resolves the name to match actor filter patterns against. + * + * GitHub's GraphQL API returns the bare login for App actors ("dependabot"), + * whereas REST and the GitHub UI use a "[bot]" suffix ("dependabot[bot]"). Users + * write filter patterns in the suffixed form, both the documented "*[bot]" + * wildcard and exact entries like "renovate[bot]", so GraphQL bot logins are + * normalized to that form before matching. Without this no "[bot]" pattern can + * ever match, because the suffix is simply absent from the data. + * + * @param author - Comment author; null for deleted ("ghost") accounts + * @returns Actor name, "[bot]"-suffixed for App actors + */ +export function resolveActorName( + author: { login: string; __typename?: string } | null | undefined, +): string { + if (!author) return "ghost"; + + if (author.__typename === "Bot" && !author.login.endsWith("[bot]")) { + return `${author.login}[bot]`; + } + + return author.login; +} + /** * Checks if an actor matches a pattern * Supports wildcards: "*[bot]" matches all bots, "dependabot[bot]" matches specific diff --git a/test/actor-filter.test.ts b/test/actor-filter.test.ts index e15cb04c..3f1a4418 100644 --- a/test/actor-filter.test.ts +++ b/test/actor-filter.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { parseActorFilter, actorMatchesPattern, + resolveActorName, shouldIncludeCommentByActor, } from "../src/github/utils/actor-filter"; @@ -170,3 +171,49 @@ describe("shouldIncludeCommentByActor", () => { ).toBe(false); }); }); + +describe("resolveActorName", () => { + test("appends the [bot] suffix to GraphQL App actors", () => { + // GraphQL returns the bare login for bots; REST would say "dependabot[bot]". + expect(resolveActorName({ __typename: "Bot", login: "dependabot" })).toBe( + "dependabot[bot]", + ); + }); + + test("leaves human logins untouched", () => { + expect(resolveActorName({ __typename: "User", login: "octocat" })).toBe( + "octocat", + ); + }); + + test("does not double-suffix a login that already ends with [bot]", () => { + expect( + resolveActorName({ __typename: "Bot", login: "dependabot[bot]" }), + ).toBe("dependabot[bot]"); + }); + + test("maps deleted accounts to ghost", () => { + expect(resolveActorName(null)).toBe("ghost"); + expect(resolveActorName(undefined)).toBe("ghost"); + }); + + test("falls back to the login when __typename is absent", () => { + expect(resolveActorName({ login: "octocat" })).toBe("octocat"); + }); + + test("a bot actor matches the *[bot] wildcard once resolved", () => { + const actor = resolveActorName({ __typename: "Bot", login: "renovate" }); + + expect(actorMatchesPattern(actor, "*[bot]")).toBe(true); + // The raw GraphQL login never matches, which is the bug being fixed. + expect(actorMatchesPattern("renovate", "*[bot]")).toBe(false); + }); + + test("a bot actor matches an exact [bot] pattern once resolved", () => { + const actor = resolveActorName({ __typename: "Bot", login: "dependabot" }); + + expect(shouldIncludeCommentByActor(actor, [], ["dependabot[bot]"])).toBe( + false, + ); + }); +}); diff --git a/test/data-fetcher.test.ts b/test/data-fetcher.test.ts index 945eb55e..5bacb14c 100644 --- a/test/data-fetcher.test.ts +++ b/test/data-fetcher.test.ts @@ -1215,7 +1215,10 @@ describe("fetchGitHubData integration with time filtering", () => { { id: "2", databaseId: "2", - author: { login: "scanner[bot]" }, + // GraphQL returns the bare login for App actors plus + // __typename: "Bot". It does NOT append a "[bot]" suffix the + // way REST does, so this mirrors a real payload. + author: { __typename: "Bot", login: "scanner" }, body: "Pre-trigger bot review", state: "COMMENTED", submittedAt: "2024-01-15T11:00:00Z",