From 45986960daef830faf7293d9a4f415be1aae29d9 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Sat, 10 Oct 2026 15:52:22 +0200 Subject: [PATCH 1/3] refactor(forgekeeper): derive RPC types from the schemas OpenCode types the values of JSON Schema RPC definitions as `unknown`, so the server and the CLI each cast them to a hand-written shape that nothing tied to the schemas. Derive the input, output, and event types from the schemas with json-schema-to-ts, a type-only dev dependency that adds nothing to the bundles. Handlers now receive typed input through one adapter, and the CLI casts to the derived types. Describe `statuses` and the snapshots as snapshots, and close the output and event schemas with additionalProperties, so they match what the server sends. --- bun.lock | 11 +++- package.json | 1 + packages/forgekeeper/src/rpc.ts | 56 +++++++++++++++---- packages/forgekeeper/src/server.ts | 46 ++++++--------- .../forgekeeper/src/status-client.test.ts | 6 +- packages/forgekeeper/src/status-client.ts | 13 ++--- packages/forgekeeper/src/tui.tsx | 22 +++----- 7 files changed, 87 insertions(+), 68 deletions(-) diff --git a/bun.lock b/bun.lock index 2d9e0f0..0e6027b 100644 --- a/bun.lock +++ b/bun.lock @@ -13,6 +13,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", @@ -42,7 +43,7 @@ }, "packages/forgekeeper": { "name": "opencode-forgekeeper", - "version": "0.0.0", + "version": "2.0.0", "peerDependencies": { "@opencode/plugin": "^2.0.26", "@opentui/core": ">=0.5.8", @@ -59,7 +60,7 @@ }, "packages/terminal-progress": { "name": "opencode-terminal-progress", - "version": "1.0.2", + "version": "1.1.0", "peerDependencies": { "@opencode/plugin": "^2.0.0", }, @@ -173,6 +174,8 @@ "@babel/preset-typescript": ["@babel/preset-typescript@7.27.1", "", { "dependencies": { "@babel/helper-plugin-utils": "^7.27.1", "@babel/helper-validator-option": "^7.27.1", "@babel/plugin-syntax-jsx": "^7.27.1", "@babel/plugin-transform-modules-commonjs": "^7.27.1", "@babel/plugin-transform-typescript": "^7.27.1" }, "peerDependencies": { "@babel/core": "^7.0.0-0" } }, "sha512-l7WfQfX0WK4M0v2RudjuQK4u99BS6yLHYEmdtVPP7lKV013zr9DygFuWNlnbvQ9LR+LS0Egz/XAvGx5U9MX0fQ=="], + "@babel/runtime": ["@babel/runtime@7.29.10", "", {}, "sha512-NpugTzbKgGrNe45Ui32cN8vCFFMxVtP/9rtfwv0nP5FRDAjvYBxJcLVpYA9mr1jxP6cnIT7x9LUzuDTbWYYNRA=="], + "@babel/template": ["@babel/template@7.29.7", "", { "dependencies": { "@babel/code-frame": "^7.29.7", "@babel/parser": "^7.29.7", "@babel/types": "^7.29.7" } }, "sha512-puq+Gf35oI24FeN11LkoUQFqv9uwNeWpxXZi/Ji3rRIoKAzKnxRaZ+Gkj0vKS9ZCiTESfng1N9LyOyXvo+m+Gg=="], "@babel/traverse": ["@babel/traverse@7.29.10", "", { "dependencies": { "@babel/code-frame": "^7.29.7", "@babel/generator": "^7.29.8", "@babel/helper-globals": "^7.29.7", "@babel/parser": "^7.29.9", "@babel/template": "^7.29.7", "@babel/types": "^7.29.8", "debug": "^4.3.1" } }, "sha512-B3lYAs2nkKskbgQ1nfy8Ma9ao9Q1rntTAQHrzzyDuj+TSO+X3le6PfWwqDvodsb+OumE10eMFfuMCTQSQ5iuIQ=="], @@ -671,6 +674,8 @@ "json-schema": ["json-schema@0.4.0", "", {}, "sha512-es94M3nTIfsEPisRafak+HDLfHXnKBhV3vU5eqPcS3flIWqcxJWgXHXiey3YrpaNsanY5ei1VoYEbOzijuq9BA=="], + "json-schema-to-ts": ["json-schema-to-ts@3.1.1", "", { "dependencies": { "@babel/runtime": "^7.18.3", "ts-algebra": "^2.0.0" } }, "sha512-+DWg8jCJG2TEnpy7kOm/7/AxaYoaRbjVB4LFZLySZlWn8exGs3A4OLJR966cVvU26N7X9TWxl+Jsw7dzAqKT6g=="], + "json-schema-traverse": ["json-schema-traverse@0.4.1", "", {}, "sha512-xbbCH5dCYU5T8LcEhhuh7HJ88HXuW3qsI3Y0zOZFKfZEHcpWiHU/Jxzk629Brsab/mMiHQti9wMP+845RPe3Vg=="], "json-stable-stringify-without-jsonify": ["json-stable-stringify-without-jsonify@1.0.1", "", {}, "sha512-Bdboy+l7tA3OGW6FjyFHWkP5LuByj1Tk33Ljyq0axyzdk9//JSi2u3fP1QSmd1KNwq6VOKYGlAu87CisVir6Pw=="], @@ -905,6 +910,8 @@ "treeverse": ["treeverse@3.0.0", "", {}, "sha512-gcANaAnd2QDZFmHFEOF4k7uc1J/6a6z3DJMd/QwEyxLoKGiptJRwid582r7QIsFlFMIZ3SnxfS52S4hm2DHkuQ=="], + "ts-algebra": ["ts-algebra@2.0.0", "", {}, "sha512-FPAhNPFMrkwz76P7cdjdmiShwMynZYN6SgOujD1urY4oNm80Ou9oMdmbR45LotcKOXoy7wSmHkRFE6Mxbrhefw=="], + "ts-api-utils": ["ts-api-utils@2.4.0", "", { "peerDependencies": { "typescript": ">=4.8.4" } }, "sha512-3TaVTaAv2gTiMB35i3FiGJaRfwb3Pyn/j3m/bfAvGe8FB7CF6u+LMYqYlDh7reQf7UNvoTvdfAqHGmPGOSsPmA=="], "tslib": ["tslib@2.8.1", "", {}, "sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w=="], diff --git a/package.json b/package.json index 93e6e7d..754521a 100644 --- a/package.json +++ b/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", diff --git a/packages/forgekeeper/src/rpc.ts b/packages/forgekeeper/src/rpc.ts index d364d63..017952b 100644 --- a/packages/forgekeeper/src/rpc.ts +++ b/packages/forgekeeper/src/rpc.ts @@ -5,6 +5,13 @@ // Status keys are session IDs, or "" for the checkout outside a session. A // key is relative to the location the call or event names, because each // checkout's server plugin instance watches its own sessions. +// +// The schemas are plain JSON Schema, so OpenCode types their values as +// `unknown`. The types at the bottom of this file derive from them instead, +// so the schemas stay the single description of what crosses the RPC. +import type { FromSchema, JSONSchema } from 'json-schema-to-ts'; +import type { Snapshot } from './store'; + const urls = { url: { type: 'string' }, issueUrl: { type: 'string' }, @@ -25,13 +32,15 @@ const target = { } as const; const key = { type: 'string' } as const; -// A `Snapshot` from src/store.ts. -const snapshot = { type: 'object' } as const; +// A `Snapshot` from src/store.ts. Its title lets `Deserialize` map it to that +// type, so the schema doesn't repeat the lookup's shape. +const snapshot = { type: 'object', title: 'Snapshot' } as const; // `enabled` is false when the plugin's `reviewStatus` option is off. const status = { type: 'object', properties: { enabled: { type: 'boolean' }, snapshot }, required: ['enabled', 'snapshot'], + additionalProperties: false, } as const; export const ForgeSessionRpc = { @@ -63,8 +72,12 @@ export const ForgeSessionRpc = { }, output: { type: 'object', - properties: { enabled: { type: 'boolean' }, statuses: { type: 'object' } }, + properties: { + enabled: { type: 'boolean' }, + statuses: { type: 'object', additionalProperties: snapshot }, + }, required: ['enabled', 'statuses'], + additionalProperties: false, }, }, // Drops every lease the CLI holds, when it exits. @@ -75,7 +88,7 @@ export const ForgeSessionRpc = { required: ['clientID'], additionalProperties: false, }, - output: { type: 'object' }, + output: { type: 'object', additionalProperties: false }, }, // Looks the key up now and returns the result. refresh: { @@ -104,6 +117,7 @@ export const ForgeSessionRpc = { type: 'object', properties: { directory: { type: 'string' }, key, snapshot }, required: ['directory', 'key', 'snapshot'], + additionalProperties: false, }, }, // A toast for the CLIs, such as after telling a session about a review. @@ -118,6 +132,7 @@ export const ForgeSessionRpc = { variant: { type: 'string', enum: ['info', 'error'] }, }, required: ['message', 'variant'], + additionalProperties: false, }, }, }, @@ -130,11 +145,28 @@ export const HOME = ''; export const HEARTBEAT = 60_000; export const LEASE_TIME = 3 * HEARTBEAT; -export type TargetUrls = { url: string; issueUrl?: string }; -export type TargetOutput = { url?: string; issueUrl?: string; targets?: TargetUrls[] }; -export type Notice = { - sessionID?: string; - title?: string; - message: string; - variant: 'info' | 'error'; -}; +type Methods = typeof ForgeSessionRpc.methods; +type Events = typeof ForgeSessionRpc.events; +type Deserialize = { pattern: { type: 'object'; title: 'Snapshot' }; output: Snapshot }; +type Typed = FromSchema; + +export type MethodName = keyof Methods; +export type EventName = keyof Events; +export type Input = Typed; +export type Output = Typed; +export type EventData = Typed; + +export type TargetOutput = Output<'target'>; +export type TargetUrls = NonNullable[number]; +export type Notice = EventData<'notice'>; + +export type Handlers = { [M in MethodName]: (input: Input) => Promise> }; + +// OpenCode checks each call against the schemas before it reaches a handler, +// and types the input as `unknown`. This is the one place that narrows it. +export const implement = (handlers: Handlers) => ({ + target: (input: unknown) => handlers.target(input as Input<'target'>), + watch: (input: unknown) => handlers.watch(input as Input<'watch'>), + release: (input: unknown) => handlers.release(input as Input<'release'>), + refresh: (input: unknown) => handlers.refresh(input as Input<'refresh'>), +}); diff --git a/packages/forgekeeper/src/server.ts b/packages/forgekeeper/src/server.ts index 19301a1..6d21ea9 100644 --- a/packages/forgekeeper/src/server.ts +++ b/packages/forgekeeper/src/server.ts @@ -2,7 +2,7 @@ import type { Plugin } from '@opencode/plugin'; import { CONCURRENCY, mapLimit } from './limit'; import type { TitleLookups } from './lookups'; import { statusOptions } from './options'; -import { ForgeSessionRpc } from './rpc'; +import { ForgeSessionRpc, implement } from './rpc'; import { setupStatus, type Status, type StatusDependencies } from './status-server'; import type { Snapshot } from './store'; import { @@ -68,7 +68,7 @@ async function drain(tasks: Promise[], limit: number) { } // RPC values must be JSON, which has no `undefined`, such as a cleared error. -const json = (value: unknown) => JSON.parse(JSON.stringify(value)) as never; +const json = (value: T): T => JSON.parse(JSON.stringify(value)); // Stores each root session's targets, keeps its title prefix current, serves // the targets to the CLI over RPC, and, given `status`, watches the PRs/MRs @@ -119,32 +119,22 @@ export async function setupServer( }; let watcher: Status | undefined; - const rpc = await ctx.rpc.register(ForgeSessionRpc, { - target: async (input) => { - const { sessionID } = input as { sessionID: string }; - return targetOutput((await stateFor(sessionID)).targets); - }, - watch: async (input) => { - const { clientID, keys, visible } = input as { - clientID: string; - keys: string[]; - visible?: string; - }; - return watcher - ? { enabled: true, statuses: json(watcher.service.watch(clientID, keys, visible)) } - : { enabled: false, statuses: {} }; - }, - release: async (input) => { - watcher?.service.release((input as { clientID: string }).clientID); - return {}; - }, - refresh: async (input) => { - const { key } = input as { key: string }; - return watcher - ? { enabled: true, snapshot: json(await watcher.service.refresh(key)) } - : DISABLED; - }, - }); + const rpc = await ctx.rpc.register( + ForgeSessionRpc, + implement({ + target: async ({ sessionID }) => targetOutput((await stateFor(sessionID)).targets), + watch: async ({ clientID, keys, visible }) => + watcher + ? { enabled: true, statuses: json(watcher.service.watch(clientID, keys, visible)) } + : { enabled: false, statuses: {} }, + release: async ({ clientID }) => { + watcher?.service.release(clientID); + return {}; + }, + refresh: async ({ key }) => + watcher ? { enabled: true, snapshot: json(await watcher.service.refresh(key)) } : DISABLED, + }), + ); const settings = statusOptions(ctx.options); if (status && settings.enabled) { diff --git a/packages/forgekeeper/src/status-client.test.ts b/packages/forgekeeper/src/status-client.test.ts index 3766604..0faf11d 100644 --- a/packages/forgekeeper/src/status-client.test.ts +++ b/packages/forgekeeper/src/status-client.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test'; -import { HOME } from './rpc'; +import { HOME, type Output } from './rpc'; import { createStatusClient } from './status-client'; import type { Snapshot } from './store'; @@ -28,7 +28,7 @@ function harness() { }, refresh: async (directory, key) => ({ enabled: enabled.get(directory) ?? true, - snapshot: statuses.get(directory)?.[key], + snapshot: statuses.get(directory)?.[key] ?? { loading: false }, }), release: async (directory) => void released.push(directory), onChange: () => {}, @@ -126,7 +126,7 @@ describe('createStatusClient', () => { }); test('keeps the newest snapshot when an older RPC response arrives after an event', async () => { - let answer = (_value: { enabled: boolean; statuses: Record }) => {}; + let answer = (_value: Output<'watch'>) => {}; const client = createStatusClient({ clientID: 'cli', watch: () => new Promise((resolve) => (answer = resolve)), diff --git a/packages/forgekeeper/src/status-client.ts b/packages/forgekeeper/src/status-client.ts index 5d8757e..3c546dc 100644 --- a/packages/forgekeeper/src/status-client.ts +++ b/packages/forgekeeper/src/status-client.ts @@ -1,4 +1,4 @@ -import { HEARTBEAT, HOME } from './rpc'; +import { HEARTBEAT, HOME, type Input, type Output } from './rpc'; import type { Snapshot } from './store'; // Where a status lives: the server plugin instance of `directory`, and a @@ -13,11 +13,8 @@ const placeID = (place: Place) => JSON.stringify([place.directory, place.key]); export interface StatusClientOptions { clientID: string; - watch: ( - directory: string, - input: { clientID: string; keys: string[]; visible?: string }, - ) => Promise<{ enabled: boolean; statuses: Record }>; - refresh: (directory: string, key: string) => Promise<{ enabled: boolean; snapshot: unknown }>; + watch: (directory: string, input: Input<'watch'>) => Promise>; + refresh: (directory: string, key: string) => Promise>; release: (directory: string, clientID: string) => Promise; // Called after any snapshot or setting changes, so the UI can render again. onChange: () => void; @@ -107,7 +104,7 @@ export function createStatusClient(options: StatusClientOptions) { failures.delete(directory); enabled.set(directory, result.enabled); for (const [key, snapshot] of Object.entries(result.statuses)) { - if (isShown({ directory, key })) apply({ directory, key }, snapshot as Snapshot); + if (isShown({ directory, key })) apply({ directory, key }, snapshot); } options.onChange(); } catch (error) { @@ -161,7 +158,7 @@ export function createStatusClient(options: StatusClientOptions) { try { const result = await options.refresh(place.directory, place.key); enabled.set(place.directory, result.enabled); - apply(place, result.snapshot as Snapshot); + apply(place, result.snapshot); options.onChange(); } catch (error) { unavailable(place.directory, error); diff --git a/packages/forgekeeper/src/tui.tsx b/packages/forgekeeper/src/tui.tsx index 02628d0..a0ea7e4 100644 --- a/packages/forgekeeper/src/tui.tsx +++ b/packages/forgekeeper/src/tui.tsx @@ -17,7 +17,7 @@ import { } from './format'; import type { ReviewRequest } from './forge'; import { reference } from './forges'; -import { ForgeSessionRpc, HEARTBEAT, HOME, type Notice } from './rpc'; +import { ForgeSessionRpc, HEARTBEAT, HOME, type EventData, type Output } from './rpc'; import { createStatusClient, samePlace, type Place } from './status-client'; import type { Snapshot } from './store'; @@ -29,16 +29,12 @@ export default { const [version, setVersion] = createSignal(0); const client = createStatusClient({ clientID: crypto.randomUUID(), + // OpenCode types the results as `unknown`; the server returns what the + // schemas in rpc.ts describe. watch: async (directory, input) => - (await rpc.watch(input, { location: { directory } })) as { - enabled: boolean; - statuses: Record; - }, + (await rpc.watch(input, { location: { directory } })) as Output<'watch'>, refresh: async (directory, key) => - (await rpc.refresh({ key }, { location: { directory } })) as { - enabled: boolean; - snapshot: unknown; - }, + (await rpc.refresh({ key }, { location: { directory } })) as Output<'refresh'>, release: (directory, clientID) => rpc.release({ clientID }, { location: { directory } }), onChange: () => setVersion((value) => value + 1), }); @@ -382,16 +378,12 @@ export default { const stops = [ rpc.events.on('status', (event) => { - const { directory, key, snapshot } = event.data as { - directory: string; - key: string; - snapshot: Snapshot; - }; + const { directory, key, snapshot } = event.data as EventData<'status'>; client.receive(directory, key, snapshot); }), // Notices about a session go to the CLIs that have shown it. rpc.events.on('notice', (event) => { - const notice = event.data as Notice; + const notice = event.data as EventData<'notice'>; if (notice.sessionID && !client.hasShown(notice.sessionID)) return; context.ui.toast.show({ ...(notice.title ? { title: notice.title } : {}), From 49b9274547dc1ee58b197695de847e001409a66f Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Sat, 10 Oct 2026 15:52:32 +0200 Subject: [PATCH 2/3] refactor(forgekeeper): type GraphQL responses by query GitLab's GraphQL transport returned `Record`, and GitHub's returned `Record`, so each call site cast the response to the shape it expected, or relied on `any`. Pair each query with the shape of the `data` it selects. The transports return `unknown`, because they can only vouch that the response parsed, and one adapter applies the query's shape. Call sites then read typed results without casts, and the `any` is gone. --- packages/forgekeeper/src/github.ts | 81 ++++++++++++--------- packages/forgekeeper/src/gitlab-feedback.ts | 24 ++++-- packages/forgekeeper/src/gitlab.test.ts | 32 +++++--- packages/forgekeeper/src/gitlab.ts | 75 +++++++++++-------- packages/forgekeeper/src/graphql.ts | 31 ++++++++ 5 files changed, 159 insertions(+), 84 deletions(-) create mode 100644 packages/forgekeeper/src/graphql.ts diff --git a/packages/forgekeeper/src/github.ts b/packages/forgekeeper/src/github.ts index 1813443..a479c8f 100644 --- a/packages/forgekeeper/src/github.ts +++ b/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 { 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, -) => Promise>; +type Variables = Record; +export type GraphQL = Transport; + +// Every query selects fields of one repository. A repository that doesn't +// exist, or that the viewer can't see, is null. +type RepositoryData = { 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; + data?: object; errors?: Array<{ type?: string; message: string }>; }; try { @@ -188,7 +190,11 @@ export const githubTraits: ForgeTraits = { export class GitHubForge implements Forge { readonly traits = githubTraits; - constructor(readonly graphql: GraphQL) {} + private readonly graphql: Typed; + + 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>( + `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> | 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> } - | 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>( + `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 { @@ -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 | 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 } | 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 | 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 } } } - | undefined - )?.object?.statusCheckRollup?.contexts; + const next = data.repository?.object?.statusCheckRollup?.contexts; if (!next) break; contexts.push(...next.nodes); cursor = next.pageInfo; diff --git a/packages/forgekeeper/src/gitlab-feedback.ts b/packages/forgekeeper/src/gitlab-feedback.ts index ae5e4ac..80c8ca3 100644 --- a/packages/forgekeeper/src/gitlab-feedback.ts +++ b/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 { + 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 = { 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); diff --git a/packages/forgekeeper/src/gitlab.test.ts b/packages/forgekeeper/src/gitlab.test.ts index f26027d..2d51a58 100644 --- a/packages/forgekeeper/src/gitlab.test.ts +++ b/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 => { + 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, diff --git a/packages/forgekeeper/src/gitlab.ts b/packages/forgekeeper/src/gitlab.ts index 2ad73d2..5957d32 100644 --- a/packages/forgekeeper/src/gitlab.ts +++ b/packages/forgekeeper/src/gitlab.ts @@ -14,6 +14,7 @@ import { } from './forge'; import type { RemoteProject, Repository } from './git'; import { fetchComments } from './gitlab-feedback'; +import { query, typed, type Query, type Transport, type Typed } from './graphql'; interface DiscussionPage { pageInfo: { hasNextPage: boolean; endCursor: string | null }; @@ -44,6 +45,13 @@ interface MergeRequestNode { reviewers?: { nodes: ReviewerNode[] } | null; } +// What `mergeRequestNumbersQuery` selects. +interface MergeRequestNumber { + iid: string; + updatedAt: string | null; + sourceProject: { fullPath: string } | null; +} + interface ReviewerNode { type: string; username?: string; @@ -65,7 +73,13 @@ const MERGE_REQUEST_FIELDS = ` discussions(first: ${PAGE_SIZE}) { ${DISCUSSIONS} } `; -export function mergeRequestsQuery(projectCount: number): string { +// Queries that look up several projects give each an alias, `p0`, `p1`, and +// so on. A project that doesn't exist, or that the viewer can't see, is null. +type Projects = { [alias: `p${number}`]: T | null | undefined }; + +export function mergeRequestsQuery( + projectCount: number, +): Query> { const variables = Array.from({ length: projectCount }, (_, index) => `$p${index}: ID!`).join( ', ', ); @@ -76,10 +90,12 @@ export function mergeRequestsQuery(projectCount: number): string { mergeRequests(sourceBranches: [$branch], state: opened, first: 20) { nodes { ${MERGE_REQUEST_FIELDS} } } }`, ).join('\n'); - return `query($branch: String!, ${variables}) {\n${projects}\n}`; + return query(`query($branch: String!, ${variables}) {\n${projects}\n}`); } -export function mergeRequestQuery(projectCount: number): string { +export function mergeRequestQuery( + projectCount: number, +): Query> { const variables = Array.from({ length: projectCount }, (_, index) => `$p${index}: ID!`).join( ', ', ); @@ -88,11 +104,13 @@ export function mergeRequestQuery(projectCount: number): string { (_, index) => `p${index}: project(fullPath: $p${index}) { mergeRequest(iid: $iid) { ${MERGE_REQUEST_FIELDS} } }`, ).join('\n'); - return `query($iid: String!, ${variables}) {\n${projects}\n}`; + return query(`query($iid: String!, ${variables}) {\n${projects}\n}`); } // Only the fields that pick the newest MR from the source project. -export function mergeRequestNumbersQuery(projectCount: number): string { +export function mergeRequestNumbersQuery( + projectCount: number, +): Query> { const variables = Array.from({ length: projectCount }, (_, index) => `$p${index}: ID!`).join( ', ', ); @@ -103,18 +121,22 @@ export function mergeRequestNumbersQuery(projectCount: number): string { mergeRequests(sourceBranches: [$branch], state: opened, first: 20) { nodes { iid updatedAt sourceProject { fullPath } } } }`, ).join('\n'); - return `query($branch: String!, ${variables}) {\n${projects}\n}`; + return query(`query($branch: String!, ${variables}) {\n${projects}\n}`); } -export const SOURCE_BRANCH_QUERY = `query($project: ID!, $iid: String!) { +export const SOURCE_BRANCH_QUERY = query<{ + project?: { mergeRequest?: { sourceBranch?: string | null } | null } | null; +}>(`query($project: ID!, $iid: String!) { project(fullPath: $project) { mergeRequest(iid: $iid) { sourceBranch } } -}`; +}`); -export const DISCUSSIONS_QUERY = `query($project: ID!, $iid: String!, $after: String) { +export const DISCUSSIONS_QUERY = query<{ + project?: { mergeRequest?: { discussions?: DiscussionPage | null } | null } | null; +}>(`query($project: ID!, $iid: String!, $after: String) { project(fullPath: $project) { mergeRequest(iid: $iid) { discussions(first: ${PAGE_SIZE}, after: $after) { ${DISCUSSIONS} } } } -}`; +}`); export function classifyError(message: string, code: number): ForgeError { const text = message.trim().replace(/^glab:\s*/, '') || `glab exited with code ${code}`; @@ -130,18 +152,11 @@ export function classifyError(message: string, code: number): ForgeError { return new ForgeError('request', text.split('\n')[0]); } -// The `data` of a GraphQL response. Each query casts its fields to the node -// types it requested. -export type GraphQLData = Record; - -export type GraphQL = ( - host: string, - query: string, - variables: Record, -) => Promise; +type Variables = Record; +export type GraphQL = Transport; interface GraphQLResponse { - data?: GraphQLData | null; + data?: object | null; errors?: { message?: unknown }[]; } @@ -238,7 +253,11 @@ function duoState(state: string): AutomatedReviewState { export class GitLabForge implements Forge { readonly traits = gitlabTraits; - constructor(readonly graphql: GraphQL) {} + private readonly graphql: Typed; + + constructor(graphql: GraphQL) { + this.graphql = typed(graphql); + } async findByBranch(repository: Repository): Promise { const { source, targets, sourceBranch } = repository; @@ -250,7 +269,7 @@ export class GitLabForge implements Forge { const seen = new Set(); const nodes: MergeRequestNode[] = []; targets.forEach((_, index) => { - for (const node of (data?.[`p${index}`]?.mergeRequests?.nodes ?? []) as MergeRequestNode[]) { + for (const node of data[`p${index}`]?.mergeRequests?.nodes ?? []) { // Another project's branch with the same name must not be reported as this branch's MR. if (node.sourceProject?.fullPath.toLowerCase() !== sourcePath) continue; const key = `${node.targetProject.fullPath}!${node.iid}`; @@ -275,11 +294,7 @@ export class GitLabForge implements Forge { ); const sourcePath = source.path.toLowerCase(); - const nodes: Array<{ - iid: string; - updatedAt: string | null; - sourceProject: { fullPath: string } | null; - }> = targets.flatMap((_, index) => data?.[`p${index}`]?.mergeRequests?.nodes ?? []); + const nodes = targets.flatMap((_, index) => data[`p${index}`]?.mergeRequests?.nodes ?? []); return ( nodes // Another project's branch with the same name must not name this branch's MR. @@ -293,7 +308,7 @@ export class GitLabForge implements Forge { project: ref.project, iid: ref.iid, }); - const branch = data?.project?.mergeRequest?.sourceBranch; + const branch = data.project?.mergeRequest?.sourceBranch; return typeof branch === 'string' && branch ? branch : undefined; } @@ -307,7 +322,7 @@ export class GitLabForge implements Forge { projects.forEach((project, index) => (variables[`p${index}`] = project.path)); const data = await this.graphql(host, mergeRequestQuery(projects.length), variables); for (let index = 0; index < projects.length; index++) { - const node: MergeRequestNode | undefined = data?.[`p${index}`]?.mergeRequest ?? undefined; + const node = data[`p${index}`]?.mergeRequest; if (node) return this.normalize(host, node); } return undefined; @@ -323,7 +338,7 @@ export class GitLabForge implements Forge { iid: node.iid, after: page.endCursor, }); - const next: DiscussionPage | undefined = data?.project?.mergeRequest?.discussions; + const next = data.project?.mergeRequest?.discussions; if (!next) break; count += countUnresolved(next); page = next.pageInfo; diff --git a/packages/forgekeeper/src/graphql.ts b/packages/forgekeeper/src/graphql.ts new file mode 100644 index 0000000..f7e551f --- /dev/null +++ b/packages/forgekeeper/src/graphql.ts @@ -0,0 +1,31 @@ +// Types for the GraphQL calls that `gh` and `glab` make. A query's text is +// paired with the shape of the response `data` it asks for, so each call site +// gets typed results, and the response shape sits next to the query that +// determines it. +declare const shape: unique symbol; + +export type Query = string & { readonly [shape]?: Data }; + +// The shape is a claim about what the server returns for this text: nothing +// checks it at runtime, so write it from the query's selection. +export const query = (text: string): Query => text as Query; + +// Runs a query and returns the response's `data`. The forges' transports can +// only vouch that it parsed, so they return `unknown`. +export type Transport = ( + host: string, + query: string, + variables: Variables, +) => Promise; + +export type Typed = ( + host: string, + query: Query, + variables: Variables, +) => Promise; + +// The one place that trusts a query's declared shape. +export const typed = + (transport: Transport): Typed => + async (host: string, query: Query, variables: Variables) => + (await transport(host, query, variables)) as Data; From 08dff02025c25f632b2e5d8af09eb5816415cd4d Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Sat, 10 Oct 2026 15:52:33 +0200 Subject: [PATCH 3/3] test(forgekeeper): wait for events instead of sleeping Tests of ordering, concurrency, and cancellation held work in flight with real timers: `Bun.sleep(1)` and `setTimeout(1)`, plus a settle helper that slept five times. They passed because the timers were usually long enough, not because anything told them the work had finished. Hold that work on promises that the test settles, and wait for the condition each test checks with `until`, which counts turns of the event loop instead of milliseconds. The ordering test now asserts the whole order, including the task that arrives during a handoff. Where a test only needs queued promise reactions to run, `flush` names that, in place of `Bun.sleep(0)`. --- .../src/human-review-watch.test.ts | 3 +- packages/forgekeeper/src/limit.test.ts | 49 +++++++++++++++---- packages/forgekeeper/src/locate.test.ts | 24 +++++++-- packages/forgekeeper/src/server.test.ts | 7 +-- .../forgekeeper/src/status-client.test.ts | 7 +-- .../forgekeeper/src/status-service.test.ts | 5 +- packages/forgekeeper/src/store.test.ts | 7 +-- packages/forgekeeper/src/testing.ts | 36 ++++++++++++++ 8 files changed, 112 insertions(+), 26 deletions(-) create mode 100644 packages/forgekeeper/src/testing.ts diff --git a/packages/forgekeeper/src/human-review-watch.test.ts b/packages/forgekeeper/src/human-review-watch.test.ts index 407dbab..64b4e23 100644 --- a/packages/forgekeeper/src/human-review-watch.test.ts +++ b/packages/forgekeeper/src/human-review-watch.test.ts @@ -14,6 +14,7 @@ import { type FeedbackLog, type SeenComments, } from './human-review-watch'; +import { until } from './testing'; const MINUTE = 60_000; const mr = { @@ -381,7 +382,7 @@ describe('createFeedbackWatcher', () => { now: () => clock, }); const first = watcher.check('ses_1', mr); - await Bun.sleep(0); + await until(() => sent.length === 1); clock += FETCH_INTERVAL; await watcher.check('ses_1', mr); release(); diff --git a/packages/forgekeeper/src/limit.test.ts b/packages/forgekeeper/src/limit.test.ts index c775424..bf58c06 100644 --- a/packages/forgekeeper/src/limit.test.ts +++ b/packages/forgekeeper/src/limit.test.ts @@ -1,12 +1,24 @@ import { describe, expect, test } from 'bun:test'; import type { Exec } from './exec'; import { createLimiter, limitExec, mapLimit } from './limit'; +import { deferred, until, type Deferred } from './testing'; describe('mapLimit', () => { test('keeps the order of the items', async () => { - expect( - await mapLimit([3, 1, 2], 2, async (value) => (await Bun.sleep(value), value * 10)), - ).toEqual([30, 10, 20]); + // The tasks finish in the order 1, 2, 3, which isn't the order of the items. + const gates = new Map([3, 1, 2].map((value) => [value, deferred()])); + const started: number[] = []; + const mapped = mapLimit([3, 1, 2], 2, async (value) => { + started.push(value); + await gates.get(value)!.promise; + return value * 10; + }); + await until(() => started.length === 2); + gates.get(1)!.resolve(); + await until(() => started.length === 3); + gates.get(2)!.resolve(); + gates.get(3)!.resolve(); + expect(await mapped).toEqual([30, 10, 20]); expect(await mapLimit([], 4, async (value) => value)).toEqual([]); }); }); @@ -17,19 +29,28 @@ describe('createLimiter', () => { let running = 0; let peak = 0; const order: number[] = []; + const gates = new Map(); const task = (id: number) => limited(async () => { peak = Math.max(peak, ++running); order.push(id); - await Bun.sleep(1); + gates.set(id, deferred()); + await gates.get(id)!.promise; running--; // A task queued as a slot frees up must still wait its turn. if (id === 1) void task(99); }); - await Promise.all([1, 2, 3, 4, 5].map(task)); - await Bun.sleep(5); + const done = Promise.all([1, 2, 3, 4, 5].map(task)); + // Each task finishes once it has started, so every one that follows + // starts as a slot frees up. + for (const id of [1, 2, 3, 4, 5, 99]) { + await until(() => gates.has(id)); + gates.get(id)!.resolve(); + } + await done; + await until(() => running === 0); expect(peak).toBe(2); - expect(order.slice(0, 5)).toEqual([1, 2, 3, 4, 5]); + expect(order).toEqual([1, 2, 3, 4, 5, 99]); }); test('frees the slot of a failed task', async () => { @@ -43,14 +64,22 @@ describe('limitExec', () => { test('shares one limit across every command', async () => { let running = 0; let peak = 0; + const gates: Deferred[] = []; const run: Exec = async () => { peak = Math.max(peak, ++running); - await Bun.sleep(1); + const gate = deferred(); + gates.push(gate); + await gate.promise; running--; return { stdout: '', stderr: '', code: 0 }; }; const limited = limitExec(run); - await Promise.all(Array.from({ length: 50 }, () => limited('glab', [], '/repo'))); + const done = Promise.all(Array.from({ length: 50 }, () => limited('glab', [], '/repo'))); + for (let finished = 0; finished < 50; finished++) { + await until(() => gates.length > finished); + gates[finished].resolve(); + } + await done; expect(peak).toBe(4); }); }); @@ -68,7 +97,7 @@ describe('limitExec cancellation', () => { const limited = limitExec(run, 1, stopped.signal); const first = limited('first', [], '/repo'); const queued = limited('queued', [], '/repo'); - await Bun.sleep(0); + await until(() => ran.length === 1); stopped.abort(); finish(); expect((await first).code).toBe(0); diff --git a/packages/forgekeeper/src/locate.test.ts b/packages/forgekeeper/src/locate.test.ts index 61901dd..64f5df9 100644 --- a/packages/forgekeeper/src/locate.test.ts +++ b/packages/forgekeeper/src/locate.test.ts @@ -10,6 +10,7 @@ import { kinds, traits, type ForgeCatalog } from './forges'; import type { RemoteProject, Repository, RepositoryLookup } from './git'; import { locate } from './locate'; import { classifyTargets } from './target'; +import { deferred, until, type Deferred } from './testing'; // Finds `found` by its own number only, like a forge that has just that PR/MR. class FakeForge implements Forge { @@ -99,10 +100,13 @@ const mergeOf = (iid: string, overrides: Partial = {}) => }) as GitLabMergeRequest; // Finds requests by number, fails for the numbers in `failing`, and records -// how many lookups ran at once. +// how many lookups ran at once. With `hold` set, each lookup waits in `gates` +// until the test releases it. class FailingForge extends FakeForge { peak = 0; - private running = 0; + hold = false; + readonly gates: Deferred[] = []; + running = 0; constructor( private readonly requests: Map, @@ -113,7 +117,11 @@ class FailingForge extends FakeForge { override async findByNumber(_projects: readonly RemoteProject[], number: string) { this.peak = Math.max(this.peak, ++this.running); - await new Promise((resolve) => setTimeout(resolve, 1)); + if (this.hold) { + const gate = deferred(); + this.gates.push(gate); + await gate.promise; + } this.running--; if (this.failing.has(number)) throw new Error('glab failed'); return this.requests.get(number); @@ -250,7 +258,15 @@ describe('locate', () => { new Map(Array.from({ length: 10 }, (_, index) => [`${index + 1}`, mergeOf(`${index + 1}`)])), ); const urls = targets(...Array.from({ length: 10 }, (_, index) => mergeOf(`${index + 1}`).url)); - const lookup = await locate(catalog({ gitlab }), { directory: '/repo', targets: urls }); + gitlab.hold = true; + const located = locate(catalog({ gitlab }), { directory: '/repo', targets: urls }); + // The lookups fill the limit, and the rest wait for slots. + await until(() => gitlab.running === 4); + for (let released = 0; released < 10; released++) { + await until(() => gitlab.gates.length > released); + gitlab.gates[released].resolve(); + } + const lookup = await located; expect(lookup).toMatchObject({ kind: 'found' }); expect(lookup.kind === 'found' && lookup.requests.map((request) => request.iid)).toEqual([ '1', diff --git a/packages/forgekeeper/src/server.test.ts b/packages/forgekeeper/src/server.test.ts index 9516d17..470d7f9 100644 --- a/packages/forgekeeper/src/server.test.ts +++ b/packages/forgekeeper/src/server.test.ts @@ -14,6 +14,7 @@ import type { TitleLookups } from './lookups'; import { FETCH_INTERVAL, SETTLE_TIME } from './human-review-watch'; import { setupServer } from './server'; import type { Snapshot } from './store'; +import { flush } from './testing'; type Tool = { execute: (input: unknown, context: { sessionID: string }) => Promise }; type Hook = (event: { @@ -131,9 +132,9 @@ const defaultSessions = () => ], ]); -const settle = async () => { - for (let index = 0; index < 5; index++) await Bun.sleep(1); -}; +// Lets the plugin finish the work that the doubles' promises release. None of +// that work waits for a timer, so one turn of the event loop is enough. +const settle = flush; // Where the server stores a pending automated-review notification. const pendingRecord = (sessionID: string, url: string) => diff --git a/packages/forgekeeper/src/status-client.test.ts b/packages/forgekeeper/src/status-client.test.ts index 0faf11d..e74cb55 100644 --- a/packages/forgekeeper/src/status-client.test.ts +++ b/packages/forgekeeper/src/status-client.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from 'bun:test'; import { HOME, type Output } from './rpc'; import { createStatusClient } from './status-client'; import type { Snapshot } from './store'; +import { flush } from './testing'; const found = (iid: string): Snapshot => ({ loading: false, @@ -43,7 +44,7 @@ function harness() { released, timers, fail: (value: boolean) => void (failing = value), - settle: () => Bun.sleep(0), + settle: () => flush(), }; } @@ -136,11 +137,11 @@ describe('createStatusClient', () => { }); const place = { directory: '/a', key: 'ses_1' }; client.show(place); - await Bun.sleep(0); + await flush(); const done = { ...found('1'), revision: 12 }; client.receive('/a', 'ses_1', done); answer({ enabled: true, statuses: { ses_1: { loading: true, revision: 11 } } }); - await Bun.sleep(0); + await flush(); expect(client.snapshot(place)).toEqual(done); // A newer response replaces it, and one without a revision never does. client.receive('/a', 'ses_1', { loading: false }); diff --git a/packages/forgekeeper/src/status-service.test.ts b/packages/forgekeeper/src/status-service.test.ts index 15ea184..91b542d 100644 --- a/packages/forgekeeper/src/status-service.test.ts +++ b/packages/forgekeeper/src/status-service.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from 'bun:test'; import type { GitLabMergeRequest, ReviewRequest } from './forge'; import { createStatusService, HOME } from './status-service'; import type { Lookup } from './store'; +import { flush } from './testing'; const mr = (overrides: Partial = {}): GitLabMergeRequest => ({ @@ -79,10 +80,10 @@ function harness({ clock = due[1].at; timers.delete(due[0]); due[1].callback(); - await Bun.sleep(0); + await flush(); } clock = end; - await Bun.sleep(0); + await flush(); }, }; } diff --git a/packages/forgekeeper/src/store.test.ts b/packages/forgekeeper/src/store.test.ts index ee80542..3a2d667 100644 --- a/packages/forgekeeper/src/store.test.ts +++ b/packages/forgekeeper/src/store.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from 'bun:test'; import type { Repository } from './git'; import { ForgeError, type ReviewRequest } from './forge'; import { createStatusStore, type Lookup } from './store'; +import { flush } from './testing'; const repository = { head: '', @@ -53,7 +54,7 @@ function harness( timers.delete(id); timer.callback(); } - await Bun.sleep(0); + await flush(); }, }; } @@ -293,7 +294,7 @@ describe('createStatusStore', () => { const first = store.refresh('/a'); store.invalidate('/a'); resolvers[0](found(2)); - await Bun.sleep(0); + await flush(); expect(loaded).toEqual([]); expect(store.get('/a').lookup).toBeUndefined(); resolvers[1](found()); @@ -342,7 +343,7 @@ describe('createStatusStore', () => { const first = store.refresh('/a'); store.invalidate('/a'); resolve(found()); - await Bun.sleep(0); + await flush(); expect(calls).toBe(2); resolve({ kind: 'none', reason: 'Detached HEAD' }); await first; diff --git a/packages/forgekeeper/src/testing.ts b/packages/forgekeeper/src/testing.ts new file mode 100644 index 0000000..e2aa374 --- /dev/null +++ b/packages/forgekeeper/src/testing.ts @@ -0,0 +1,36 @@ +// Helpers for tests that need to control when asynchronous work finishes. + +export interface Deferred { + promise: Promise; + resolve: (value: T) => void; + reject: (error: unknown) => void; +} + +// A promise that the test settles itself, which holds work in flight until +// the test has seen what it needs to check. +export function deferred(): Deferred { + let resolve!: (value: T) => void; + let reject!: (error: unknown) => void; + const promise = new Promise((done, fail) => { + resolve = done; + reject = fail; + }); + return { promise, resolve, reject }; +} + +// Lets every promise reaction that is already queued run, including those +// that the reactions queue in turn. It waits for no time, and for nothing +// that needs a timer or I/O, so it makes tests deterministic only for work +// built from promises. Prefer `until` when the test knows what to wait for. +export const flush = () => new Promise((resolve) => setImmediate(resolve)); + +// Waits until the condition holds. Gives up after a number of turns instead of +// an amount of time, so a slow machine can't fail it, and a condition that +// never comes true fails fast with a clear error. +export async function until(condition: () => boolean, turns = 1_000): Promise { + for (let turn = 0; turn < turns; turn++) { + if (condition()) return; + await flush(); + } + throw new Error(`Condition not met after ${turns} turns: ${condition}`); +}