refactor(forgekeeper): derive RPC types, type GraphQL responses, and gate tests on events

package.json

@@ -14,6 +14,7 @@
     "eslint": "^9.39.1",
     "eslint-config-prettier": "10.1.8",
     "eslint-plugin-prettier": "^5.1.3",
+    "json-schema-to-ts": "^3.1.1",
     "prettier": "3.8.1",
     "solid-js": "^1.9.9",
     "typescript": "^5.7.0",

packages/forgekeeper/src/github.ts

@@ -13,6 +13,7 @@ import {
 } from './forge';
 import type { RemoteProject, Repository } from './git';
 import { gitlabTraits } from './gitlab';
+import { query, typed, type Transport, type Typed } from './graphql';
 
 interface Connection<T> {
   nodes: T[];
@@ -61,11 +62,12 @@ const fields = `number title url state isDraft mergeable reviewDecision headRefO
 // Pagination stops after this many pages; results are then marked partial.
 const MAX_PAGES = 50;
 
-export type GraphQL = (
-  host: string,
-  query: string,
-  variables: Record<string, string | number>,
-) => Promise<Record<string, unknown>>;
+type Variables = Record<string, string | number>;
+export type GraphQL = Transport<Variables>;
+
+// Every query selects fields of one repository. A repository that doesn't
+// exist, or that the viewer can't see, is null.
+type RepositoryData<T> = { repository?: T | null };
 
 export function ghGraphQL(run: Exec, cwd: string): GraphQL {
   return async (host, query, variables) => {
@@ -74,7 +76,7 @@ export function ghGraphQL(run: Exec, cwd: string): GraphQL {
       args.push(typeof value === 'number' ? '-F' : '-f', `${name}=${value}`);
     const result = await run('gh', args, cwd);
     let body: {
-      data?: Record<string, unknown>;
+      data?: object;
       errors?: Array<{ type?: string; message: string }>;
     };
     try {
@@ -188,7 +190,11 @@ export const githubTraits: ForgeTraits<GitHubPullRequest, GitHubForge> = {
 export class GitHubForge implements Forge {
   readonly traits = githubTraits;
 
-  constructor(readonly graphql: GraphQL) {}
+  private readonly graphql: Typed<Variables>;
+
+  constructor(graphql: GraphQL) {
+    this.graphql = typed(graphql);
+  }
 
   async findByNumber(
     projects: readonly RemoteProject[],
@@ -198,12 +204,14 @@ export class GitHubForge implements Forge {
       const [owner, repo] = project.path.split('/');
       const data = await this.graphql(
         project.host,
-        `query($owner:String!,$repo:String!,$number:Int!) {
+        query<RepositoryData<{ pullRequest?: Pull | null }>>(
+          `query($owner:String!,$repo:String!,$number:Int!) {
         repository(owner:$owner,name:$repo) { pullRequest(number:$number) { ${fields} } }
       }`,
+        ),
         { owner, repo, number: Number(number) },
       );
-      const node = (data.repository as { pullRequest?: Pull } | undefined)?.pullRequest;
+      const node = data.repository?.pullRequest;
       if (node) return this.normalize(project.host, node);
     }
     return undefined;
@@ -215,18 +223,17 @@ export class GitHubForge implements Forge {
       const [owner, repo] = target.path.split('/');
       const data = await this.graphql(
         target.host,
-        `query($owner:String!,$repo:String!,$branch:String!) {
+        query<
+          RepositoryData<{
+            pullRequests?: Connection<Pick<Pull, 'number' | 'updatedAt' | 'headRepository'>> | null;
+          }>
+        >(`query($owner:String!,$repo:String!,$branch:String!) {
         repository(owner:$owner,name:$repo) { pullRequests(first:100,states:OPEN,headRefName:$branch,orderBy:{field:UPDATED_AT,direction:DESC}) {
           nodes { number updatedAt headRepository { nameWithOwner } }
-        } } }`,
+        } } }`),
         { owner, repo, branch: repository.sourceBranch },
       );
-      const nodes =
-        (
-          data.repository as
-            | { pullRequests?: Connection<Pick<Pull, 'number' | 'updatedAt' | 'headRepository'>> }
-            | undefined
-        )?.pullRequests?.nodes ?? [];
+      const nodes = data.repository?.pullRequests?.nodes ?? [];
       for (const node of nodes) {
         // A fork's branch with the same name must not name this branch's PR.
         if (
@@ -243,15 +250,14 @@ export class GitHubForge implements Forge {
     const [owner, repo] = ref.project.split('/');
     const data = await this.graphql(
       ref.host,
-      `query($owner:String!,$repo:String!,$number:Int!) {
+      query<RepositoryData<{ pullRequest?: { headRefName?: string | null } | null }>>(
+        `query($owner:String!,$repo:String!,$number:Int!) {
       repository(owner:$owner,name:$repo) { pullRequest(number:$number) { headRefName } }
     }`,
+      ),
       { owner, repo, number: Number(ref.iid) },
     );
-    const branch = (
-      data.repository as { pullRequest?: { headRefName?: unknown } | null } | undefined
-    )?.pullRequest?.headRefName;
-    return typeof branch === 'string' && branch ? branch : undefined;
+    return data.repository?.pullRequest?.headRefName || undefined;
   }
 
   async findByBranch(repository: Repository): Promise<GitHubPullRequest[]> {
@@ -263,14 +269,15 @@ export class GitHubForge implements Forge {
       for (let page = 0; page < MAX_PAGES; page++) {
         const data = await this.graphql(
           target.host,
-          `query($owner:String!,$repo:String!,$branch:String!${after ? ',$after:String!' : ''}) {
+          query<RepositoryData<{ pullRequests?: Connection<Pull> | null }>>(
+            `query($owner:String!,$repo:String!,$branch:String!${after ? ',$after:String!' : ''}) {
           repository(owner:$owner,name:$repo) { pullRequests(first:100,states:OPEN,headRefName:$branch${after ? ',after:$after' : ''}) {
             nodes { ${fields} } pageInfo { hasNextPage endCursor }
           } } }`,
+          ),
           { owner, repo, branch: repository.sourceBranch, ...(after ? { after } : {}) },
         );
-        const pulls = (data.repository as { pullRequests?: Connection<Pull> } | undefined)
-          ?.pullRequests;
+        const pulls = data.repository?.pullRequests;
         if (!pulls) throw new ForgeError('request', 'GitHub repository unavailable');
         for (const node of pulls.nodes) {
           // A fork's branch with the same name must not be reported as this branch's PR.
@@ -301,15 +308,17 @@ export class GitHubForge implements Forge {
     for (let count = 1; page?.hasNextPage && page.endCursor && count < MAX_PAGES; count++) {
       const data = await this.graphql(
         host,
-        `query($owner:String!,$repo:String!,$number:Int!,$after:String!) {
+        query<
+          RepositoryData<{ pullRequest?: { reviewThreads?: Pull['reviewThreads'] | null } | null }>
+        >(
+          `query($owner:String!,$repo:String!,$number:Int!,$after:String!) {
         repository(owner:$owner,name:$repo) { pullRequest(number:$number) {
           reviewThreads(first:100,after:$after) { nodes { isResolved } pageInfo { hasNextPage endCursor } }
         } } }`,
+        ),
         { owner, repo, number: node.number, after: page.endCursor },
       );
-      const next = (
-        data.repository as { pullRequest?: { reviewThreads?: Pull['reviewThreads'] } } | undefined
-      )?.pullRequest?.reviewThreads;
+      const next = data.repository?.pullRequest?.reviewThreads;
       if (!next) break;
       threads.push(...next.nodes);
       page = next.pageInfo;
@@ -328,17 +337,17 @@ export class GitHubForge implements Forge {
     for (let count = 1; cursor?.hasNextPage && cursor.endCursor && count < MAX_PAGES; count++) {
       const data = await this.graphql(
         host,
-        `query($owner:String!,$repo:String!,$oid:GitObjectID!,$after:String!) {
+        query<
+          RepositoryData<{
+            object?: { statusCheckRollup?: { contexts?: Connection<Check> | null } | null } | null;
+          }>
+        >(`query($owner:String!,$repo:String!,$oid:GitObjectID!,$after:String!) {
         repository(owner:$owner,name:$repo) { object(oid:$oid) { ... on Commit { statusCheckRollup {
           contexts(first:100,after:$after) { nodes { __typename ... on CheckRun { status conclusion detailsUrl } ... on StatusContext { state targetUrl } } pageInfo { hasNextPage endCursor } }
-        } } } } }`,
+        } } } } }`),
         { owner, repo, oid: node.headRefOid, after: cursor.endCursor },
       );
-      const next = (
-        data.repository as
-          | { object?: { statusCheckRollup?: { contexts?: Connection<Check> } } }
-          | undefined
-      )?.object?.statusCheckRollup?.contexts;
+      const next = data.repository?.object?.statusCheckRollup?.contexts;
       if (!next) break;
       contexts.push(...next.nodes);
       cursor = next.pageInfo;

packages/forgekeeper/src/gitlab-feedback.ts

@@ -1,6 +1,7 @@
 // Reads a GitLab MR's comments for GitLab's `feedback` capability.
 import type { FeedbackSnapshot, ReviewComment } from './forge';
 import type { GraphQL } from './gitlab';
+import { query, typed } from './graphql';
 
 interface NoteNode {
   id: string;
@@ -20,7 +21,17 @@ export const MAX_COMMENT_PAGES = 20;
 
 // ONLY_COMMENTS leaves out system notes such as "added 1 commit". Pages run
 // backwards from the newest comment, in creation order within each page.
-export const COMMENTS_QUERY = `query($project: ID!, $iid: String!, $before: String) {
+export const COMMENTS_QUERY = query<{
+  currentUser?: { id: string } | null;
+  project?: {
+    mergeRequest?: {
+      notes?: {
+        pageInfo: { hasPreviousPage: boolean; startCursor: string | null };
+        nodes: NoteNode[];
+      } | null;
+    } | null;
+  } | null;
+}>(`query($project: ID!, $iid: String!, $before: String) {
   currentUser { id }
   project(fullPath: $project) {
     mergeRequest(iid: $iid) {
@@ -34,7 +45,7 @@ export const COMMENTS_QUERY = `query($project: ID!, $iid: String!, $before: Stri
       }
     }
   }
-}`;
+}`);
 
 // GitLab's global ID prefix changes with the note's type, for example when a
 // reply turns a comment into a discussion, so only the number identifies it.
@@ -66,11 +77,12 @@ export function toComment(node: NoteNode): ReviewComment | undefined {
 // Reads the MR's newest comments, oldest first, or undefined when the MR
 // doesn't exist.
 export async function fetchComments(
-  graphql: GraphQL,
+  transport: GraphQL,
   host: string,
   project: string,
   iid: string,
 ): Promise<FeedbackSnapshot | undefined> {
+  const graphql = typed(transport);
   const pages: ReviewComment[][] = [];
   let viewerID: string | undefined;
   let before: string | undefined;
@@ -84,10 +96,10 @@ export async function fetchComments(
     const variables: Record<string, string> = { project, iid };
     if (before) variables.before = before;
     const data = await graphql(host, COMMENTS_QUERY, variables);
-    viewerID ??= data?.currentUser?.id ?? undefined;
-    const connection = data?.project?.mergeRequest?.notes;
+    viewerID ??= data.currentUser?.id ?? undefined;
+    const connection = data.project?.mergeRequest?.notes;
     if (!connection) return page === 0 ? undefined : snapshot(false);
-    pages.push((connection.nodes as NoteNode[]).flatMap((node) => toComment(node) ?? []));
+    pages.push(connection.nodes.flatMap((node) => toComment(node) ?? []));
     if (!connection.pageInfo.hasPreviousPage) return snapshot(true);
     before = connection.pageInfo.startCursor ?? undefined;
     if (!before) return snapshot(false);

packages/forgekeeper/src/gitlab.test.ts

@@ -399,6 +399,16 @@ describe('glabGraphQL', () => {
     (stdout: string, stderr = '', code = 0): Exec =>
     async () => ({ stdout, stderr, code });
 
+  // The error that the lookup throws.
+  const failure = async (exec: Exec): Promise<ForgeError> => {
+    try {
+      await glabGraphQL(exec, '/repo')('gitlab.com', 'q', {});
+    } catch (error) {
+      return error as ForgeError;
+    }
+    throw new Error('Expected the lookup to fail');
+  };
+
   test('passes variables as raw strings', async () => {
     let seen: string[] = [];
     const exec: Exec = async (_file, args) => {
@@ -421,30 +431,28 @@ describe('glabGraphQL', () => {
   });
 
   test('classifies failures', async () => {
-    const fail = (exec: Exec) =>
-      glabGraphQL(exec, '/repo')('gitlab.com', 'q', {}).catch((error) => error);
-    expect((await fail(run('', 'glab: 401 Unauthorized', 1))).kind).toBe('auth');
-    expect((await fail(run('', 'spawn glab ENOENT', -1))).kind).toBe('missing-glab');
-    expect((await fail(run('', 'glab: 429 Too Many Requests', 1))).kind).toBe('rate-limit');
-    const graphqlError = await fail(run('{"errors":[{"message":"Field missing"}]}', '', 1));
+    expect((await failure(run('', 'glab: 401 Unauthorized', 1))).kind).toBe('auth');
+    expect((await failure(run('', 'spawn glab ENOENT', -1))).kind).toBe('missing-glab');
+    expect((await failure(run('', 'glab: 429 Too Many Requests', 1))).kind).toBe('rate-limit');
+    const graphqlError = await failure(run('{"errors":[{"message":"Field missing"}]}', '', 1));
     expect(graphqlError).toBeInstanceOf(ForgeError);
     expect(graphqlError.message).toBe('Field missing');
   });
 
   test('fails the lookup when a field errored, even next to data', async () => {
-    const fail = (exec: Exec) =>
-      glabGraphQL(exec, '/repo')('gitlab.com', 'q', {}).catch((error) => error);
     // A timed-out field is `null` next to its error, which isn't a missing MR.
     const partial = '{"data":{"p0":null},"errors":[{"message":"request timed out","path":["p0"]}]}';
-    expect(await fail(run(partial))).toMatchObject({
+    expect(await failure(run(partial))).toMatchObject({
       kind: 'request',
       message: 'request timed out',
     });
-    expect(await fail(run('{"data":{"p0":null}}', 'glab: connection reset', 1))).toMatchObject({
+    expect(await failure(run('{"data":{"p0":null}}', 'glab: connection reset', 1))).toMatchObject({
       message: 'connection reset',
     });
-    expect(await fail(run('{"data":null}'))).toMatchObject({ message: 'GitLab returned no data' });
-    expect(await fail(run('[1, 2]'))).toBeInstanceOf(ForgeError);
+    expect(await failure(run('{"data":null}'))).toMatchObject({
+      message: 'GitLab returned no data',
+    });
+    expect(await failure(run('[1, 2]'))).toBeInstanceOf(ForgeError);
     // A project or MR that doesn't exist is `null` without an error.
     expect(await glabGraphQL(run('{"data":{"p0":null}}'), '/repo')('gitlab.com', 'q', {})).toEqual({
       p0: null,