diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index c16e77a4..c303851a 100644 --- a/docs/implementation/guarded-merge.md +++ b/docs/implementation/guarded-merge.md @@ -10,7 +10,7 @@ Merging blocks when any plan item is unreviewed or stale, an attributed file is The GitHub adapter reads the current PR base/head, effective branch rulesets, classic branch protection, required check contexts and app identities, and issue cross-references. It ignores review records that are not required checks. Cross-referenced PRs are read in one GraphQL request; repository identity is preserved with each PR number, and cross-repository references fail closed. More than 100 references also fail closed as unknown. Malformed pagination or PR references, partial referenced-PR records, rules without a type, malformed protection objects, missing or malformed check-app identities, and inconsistent check status/conclusion pairs fail closed, while an explicit `null` remains an unbound check. Display status is cached for five seconds and the combined inspection has a 12-second deadline. Each of the two sequential merge-validation inspections has a six-second deadline, keeping their combined validation budget below the 15-second serving request budget. A deadline aborts the underlying GitHub CLI process before releasing the shared in-flight request. An unreadable or incomplete rule source, a pending/missing/failing check, another open or merged PR for the issue, a conflict, or a closed PR blocks merging. -Automatic merging also requires strict required status checks as a server-enforced current-base policy. A client-side fetch cannot supply that guarantee. Merge-queue branches were blocked in this increment because `gh pr merge` can report a successful enqueue before the pull request is merged. Queue support has since landed (#24; see `merge-queue.md`): a queue-capable adapter tracks each attempt until GitHub confirms the merge, removal or failure, and queue rules count as the server-side current-base guard. An adapter without queue inspection still fails closed with a `merge-queue` blocker. The coordinator performs the complete gate twice without using the display cache, verifies the same base/head pair, then re-reads the local review generation immediately before invoking `gh pr merge --match-head-commit` with literal argv. The adapter invalidates its pre-action cache before the command and after either success or refusal. A generation guard prevents inspections started before or during the command from restoring stale cache entries afterward. After command success, the server returns “merge submitted” even if its best-effort status reload fails, and the browser keeps the action disabled until Refresh confirms GitHub state. GitHub's command refusal text is returned to the local UI, and a failed attempt also remains disabled until Refresh loads fresh state. There is no “merge anyway” path around missing atomic protection. Demo mode never constructs the coordinator, including when a gateway is injected. +Automatic merging also requires strict required status checks as a server-enforced current-base policy. A client-side fetch cannot supply that guarantee. Merge-queue branches were blocked in this increment because `gh pr merge` can report a successful enqueue before the pull request is merged. Queue support has since landed (#24; see `merge-queue.md`): a queue-capable adapter tracks each attempt until GitHub confirms the merge, removal or failure, and queue rules count as the server-side current-base guard. An adapter without queue inspection still fails closed with a `merge-queue` blocker. The coordinator performs the complete gate twice without using the display cache, verifies the same base/head pair, then re-reads the local review generation immediately before invoking `gh pr merge --match-head-commit` with literal argv. Each `gh` process gets only the allowlisted variables in `github/gh-env.ts`, plus fixed settings that turn off prompts, the pager, colour and update checks. No other server variables are passed on. A timeout or abort sends the `gh` process SIGTERM, then SIGKILL after a quarter of a second, and the call returns only after that process has exited (or 0.15 s after it exits, if a process it started keeps its output open). So a merge click aborted at its 14-second deadline settles by 14.4 s, inside the 14.5-second shutdown drain and below the 15-second request budget. Stopping `gh` cannot recall a merge request it had already sent: GitHub can still apply it, so a cancelled merge is reported as unknown and its attempt stays in flight, with the action disabled. A direct attempt is settled only when GitHub shows the pull request merged; a queued attempt also settles when GitHub reports it removed or failed (`merge-queue.md`). The adapter invalidates its pre-action cache before the command and after either success or refusal. A generation guard prevents inspections started before or during the command from restoring stale cache entries afterward. After command success, the server returns “merge submitted” even if its best-effort status reload fails, and the browser keeps the action disabled until Refresh confirms GitHub state. GitHub's command refusal text is returned to the local UI, and a failed attempt also remains disabled until Refresh loads fresh state. There is no “merge anyway” path around missing atomic protection. Demo mode never constructs the coordinator, including when a gateway is injected. Shutdown sets a terminal admission flag before inspecting active work and checks it again after partial request bodies are received. It then stops HTTP admission, aborts and awaits an active merge CLI process, drains requests, and closes the review service. A request admitted before shutdown cannot start a new merge afterward, and a cancelled command cannot outlive the local state that authorized it. diff --git a/docs/implementation/issue-prioritization.md b/docs/implementation/issue-prioritization.md index 40f7f53b..99406702 100644 --- a/docs/implementation/issue-prioritization.md +++ b/docs/implementation/issue-prioritization.md @@ -50,6 +50,15 @@ dedicated read-only gateway with these boundaries: - Codeboost invokes `gh` with literal arguments; issue text is parsed only as data and is never interpolated into a shell command or prompt. +- Each `gh` process gets only the allowlisted variables in + `github/gh-env.ts`, plus fixed settings that turn off prompts, the pager, + colour and update checks. Other variables from the server, such as + unrelated credentials, are not passed on. +- When the fetch deadline passes or the fetch is cancelled, the `gh` process + gets SIGTERM, then SIGKILL after half a second. The fetch returns only after + that process has exited, or a quarter of a second after it exits if a process + it started keeps its output open. So a fetch aborted at its 12-second + deadline settles by 12.75 s, below the 15-second request timeout. - The gateway fetches open issues and the repository's current collaborators, excludes pull requests, follows bounded pagination for both collections, and validates every field used for normalization, trust, or ranking. If the diff --git a/github/issues.ts b/github/issues.ts index a3d3e911..b04d6059 100644 --- a/github/issues.ts +++ b/github/issues.ts @@ -1,7 +1,6 @@ -import { execFile } from 'node:child_process'; -import { promisify } from 'node:util'; +import { ghEnvironment } from './gh-env.ts'; +import { runWithInput } from './run-with-input.ts'; -const runFile = promisify(execFile); const PAGE_SIZE = 100; const MAX_PAGES = 10; const MAX_ISSUES = PAGE_SIZE * MAX_PAGES; @@ -9,6 +8,12 @@ const MAX_COLLABORATORS = PAGE_SIZE * MAX_PAGES; const MAX_BODY_LENGTH = 65_536; // Covers one bounded 100-record page, including JSON-escaped bodies, labels and response overhead. export const ISSUE_PAGE_MAX_BYTES = 64 * 1024 * 1024; +/** + * How long a stopped `gh` gets after SIGTERM before SIGKILL, and how long its inherited output pipes may stay open after + * it exits. A fetch settles only when gh has stopped, so a fetch aborted at its 12 s deadline (web/issues.ts) settles up + * to 0.75 s later: 12.75 s, below the 15-second serving request budget. + */ +export const ISSUE_KILL_GRACE_MS = 500, ISSUE_PIPE_GRACE_MS = 250; export type IssueAuthorAssociation = | 'OWNER' | 'MEMBER' | 'COLLABORATOR' | 'CONTRIBUTOR' @@ -153,10 +158,13 @@ export class GhIssueGateway implements IssueGateway { constructor(repository: string, run?: RunGh, now: () => Date = () => new Date()) { if (!repositoryName(repository)) throw new Error('A GitHub repository is required for issue retrieval.'); this.repository = repository; - this.run = run ?? (async (args, options) => (await runFile('gh', [...args], { + this.run = run ?? ((args, options) => runWithInput('gh', args, { maxBuffer: ISSUE_PAGE_MAX_BYTES, signal: options?.signal, - })).stdout); + env: ghEnvironment(), + killGraceMs: ISSUE_KILL_GRACE_MS, + pipeGraceMs: ISSUE_PIPE_GRACE_MS, + })); this.now = now; } diff --git a/github/merge.ts b/github/merge.ts index 0a0596dd..81b5d58c 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,7 +1,15 @@ -import { execFile } from 'node:child_process'; -import { promisify } from 'node:util'; +import { ghEnvironment } from './gh-env.ts'; +import { runWithInput } from './run-with-input.ts'; -const runFile = promisify(execFile); +/** + * How long a stopped `gh` gets after SIGTERM before SIGKILL, and how long its inherited output pipes may stay open after + * it exits. Every gh call of this gateway settles only when gh has stopped, so a call aborted at a deadline settles up + * to 0.4 s later. That keeps a merge click (14 s deadline in runner/merge.ts) within 14.4 s, inside the 14.5 s shutdown + * drain and below the 15-second serving request budget, and an inspection (at most 12 s) within 12.4 s. + */ +export const MERGE_KILL_GRACE_MS = 250, MERGE_PIPE_GRACE_MS = 150; +/** The longest deadline of any inspection, and the default for merge-state and queue-state inspections. */ +export const MERGE_INSPECTION_TIMEOUT_MS = 12_000; export interface RequiredCheck { context: string; @@ -87,7 +95,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { throw new Error('A GitHub repository, pull request, and issue are required for merging.'); if (config.method !== undefined && !['merge','squash','rebase'].includes(config.method)) throw new Error('GitHub merge method must be merge, squash, or rebase.'); this.config = config; - this.run = run ?? (async (args, options) => (await runFile('gh', [...args], { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal })).stdout); + this.run = run ?? ((args, options) => runWithInput('gh', args, { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal, env: ghEnvironment(), killGraceMs: MERGE_KILL_GRACE_MS, pipeGraceMs: MERGE_PIPE_GRACE_MS })); } async #json(args: readonly string[], signal?: AbortSignal): Promise { @@ -240,8 +248,8 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { } async inspect(options: { fresh?: boolean; timeoutMs?: number; signal?: AbortSignal } = {}): Promise { - const timeoutMs = options.timeoutMs ?? 12_000; - if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > 12_000) throw new Error('Invalid GitHub inspection timeout.'); + const timeoutMs = options.timeoutMs ?? MERGE_INSPECTION_TIMEOUT_MS; + if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > MERGE_INSPECTION_TIMEOUT_MS) throw new Error('Invalid GitHub inspection timeout.'); if (!options.fresh && this.#cache && this.#cache.expiresAt > Date.now()) return this.#cache.state; const generation = this.#generation; if (!options.fresh && this.#inflight?.generation === generation) return this.#inflight.promise; @@ -263,7 +271,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { async queueWatermark(expectedHead: string, options: { signal?: AbortSignal; timeoutMs?: number } = {}): Promise { fullSha(expectedHead, 'expected head SHA'); const timeoutMs = options.timeoutMs ?? 6_000; - if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > 12_000) throw new Error('Invalid GitHub queue watermark timeout.'); + if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > MERGE_INSPECTION_TIMEOUT_MS) throw new Error('Invalid GitHub queue watermark timeout.'); const [owner, name] = this.config.repository.split('/') as [string, string]; const query = `query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){number headRefOid timelineItems(last:1,itemTypes:[ADDED_TO_MERGE_QUEUE_EVENT,REMOVED_FROM_MERGE_QUEUE_EVENT]){edges{cursor node{id}}}}}}`; const timeout = new AbortController(); @@ -298,8 +306,8 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway { async inspectQueue(expectedHead: string, options: { signal?: AbortSignal; timeoutMs?: number; afterCursor?: string | null } = {}): Promise { fullSha(expectedHead, 'expected head SHA'); - const timeoutMs = options.timeoutMs ?? 12_000; - if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > 12_000) throw new Error('Invalid GitHub queue inspection timeout.'); + const timeoutMs = options.timeoutMs ?? MERGE_INSPECTION_TIMEOUT_MS; + if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > MERGE_INSPECTION_TIMEOUT_MS) throw new Error('Invalid GitHub queue inspection timeout.'); const correlated = Object.hasOwn(options, 'afterCursor'); if (options.afterCursor !== undefined && options.afterCursor !== null && (typeof options.afterCursor !== 'string' || !options.afterCursor || options.afterCursor.length > 512)) throw new Error('Invalid merge-queue event cursor.'); diff --git a/runner/merge.ts b/runner/merge.ts index f255fb12..a27b8858 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -1,11 +1,13 @@ import type { ReviewService } from './review.ts'; import { mergeActionResponse, type MergeAttempt } from './store.ts'; import { ActionIdReused, GuardRefusal, MERGEABLE_STATUSES, ShuttingDownError, assertUuidV4, settleWith, type ShutdownCapability } from './lifecycle.ts'; -import { MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type MergeResult, type RemoteMergeState } from '../github/merge.ts'; +import { MERGE_INSPECTION_TIMEOUT_MS, MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type MergeResult, type RemoteMergeState } from '../github/merge.ts'; type ReviewView = ReturnType; type QueueGateway = MergeGateway & MergeQueueGateway; export interface MergeBlocker { code: string; message: string; } +/** The longest a merge click may run before it is aborted; its last gh call's stop wait comes on top (github/merge.ts). */ +export const MERGE_OPERATION_TIMEOUT_MS = 14_000; const storageError = (error: unknown) => (error as { code?: string } | null)?.code === 'ERR_SQLITE_ERROR'; /** The merge was not applied for a passing reason (deadline, shutdown); the same click may be sent again. */ export class MergeNotApplied extends Error {} @@ -44,8 +46,8 @@ export class MergeCoordinator { readonly operationTimeoutMs: number; /** Settlement of an irreversible merge keeps its writes after the Store gate closes; request-path reconciliation does not. */ #settle: (fn: () => T) => T; - constructor(service: ReviewService, gateway: MergeGateway, operationTimeoutMs = 14_000, capability?: ShutdownCapability) { - if (!Number.isSafeInteger(operationTimeoutMs) || operationTimeoutMs < 1 || operationTimeoutMs > 14_000) throw new Error('Invalid merge operation deadline.'); + constructor(service: ReviewService, gateway: MergeGateway, operationTimeoutMs = MERGE_OPERATION_TIMEOUT_MS, capability?: ShutdownCapability) { + if (!Number.isSafeInteger(operationTimeoutMs) || operationTimeoutMs < 1 || operationTimeoutMs > MERGE_OPERATION_TIMEOUT_MS) throw new Error('Invalid merge operation deadline.'); this.service = service; this.gateway = gateway; this.operationTimeoutMs = operationTimeoutMs; this.#settle = settleWith(capability); } @@ -361,7 +363,7 @@ export class MergeCoordinator { async #pollQueue(attempt: MergeAttempt, signal: AbortSignal): Promise { try { - const observation = await (this.gateway as QueueGateway).inspectQueue(attempt.reviewedHead, { signal, timeoutMs: 12_000, afterCursor: attempt.queueWatermark ?? null }); + const observation = await (this.gateway as QueueGateway).inspectQueue(attempt.reviewedHead, { signal, timeoutMs: MERGE_INSPECTION_TIMEOUT_MS, afterCursor: attempt.queueWatermark ?? null }); this.#publishQueueObservation(attempt, observation); return this.#queueStatus(); } catch (error) { diff --git a/test/gh-env.test.ts b/test/gh-env.test.ts new file mode 100644 index 00000000..5d58b173 --- /dev/null +++ b/test/gh-env.test.ts @@ -0,0 +1,121 @@ +import { chmodSync, existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it, onTestFinished } from 'vitest'; +import { GhMergeGateway, MERGE_INSPECTION_TIMEOUT_MS, MERGE_KILL_GRACE_MS, MERGE_PIPE_GRACE_MS, type RunGh } from '../github/merge.ts'; +import { GhIssueGateway, ISSUE_KILL_GRACE_MS, ISSUE_PIPE_GRACE_MS } from '../github/issues.ts'; +import { GhPullRequestGateway } from '../github/pull-requests.ts'; +import { GhAlreadyFixedGateway } from '../github/already-fixed.ts'; +import { GH_ENV_ALLOWLIST, ghEnvironment } from '../github/gh-env.ts'; +import { MERGE_OPERATION_TIMEOUT_MS } from '../runner/merge.ts'; +import { REFRESH_TIMEOUT_MS } from '../web/issues.ts'; +import { MAX_SHUTDOWN_DRAIN_MS } from '../web/server.ts'; + +// Every adapter's own runner, not an injected one: these are the runners the server uses. +// Each adapter must send every `gh` call through `run`, or this test does not see it. +const defaultRunners: [string, () => RunGh][] = [ + ['merge', () => new GhMergeGateway({ repository: 'owner/repo', pullRequest: 1, issue: 1 }).run], + ['issues', () => new GhIssueGateway('owner/repo').run], + ['pull requests', () => new GhPullRequestGateway({ repository: 'owner/repo' }).run], + ['already fixed', () => new GhAlreadyFixedGateway({ repository: 'owner/repo' }).run], +]; + +describe('default gh runners', () => { + let dir = ''; + // A known value for every allowlisted variable, so the test can check that each one reaches gh. + const expected: Record = {}; + const saved = new Map(); + const setEnv = (name: string, value: string | undefined) => { + if (!saved.has(name)) saved.set(name, process.env[name]); + if (value === undefined) delete process.env[name]; else process.env[name] = value; + }; + beforeAll(() => { + // A `gh` first on PATH that prints the environment it was given. + dir = mkdtempSync(join(tmpdir(), 'codeboost-gh-env-')); + writeFileSync(join(dir, 'gh'), '#!/bin/sh\nexec env\n'); + chmodSync(join(dir, 'gh'), 0o755); + for (const name of GH_ENV_ALLOWLIST) { + if (name === 'PATH') expected[name] = `${dir}:${process.env.PATH ?? ''}`; + else if (name === 'LANG' || name === 'LC_ALL') expected[name] = 'C'; + else expected[name] = `sentinel-${name}`; + setEnv(name, expected[name]); + } + setEnv('CODEBOOST_UNRELATED_SECRET', 'must-not-reach-gh'); + }); + afterAll(() => { + for (const [name, value] of saved) if (value === undefined) delete process.env[name]; else process.env[name] = value; + rmSync(dir, { recursive: true, force: true }); + }); + + it.each(defaultRunners)('%s: passes the allowlisted variables and nothing unrelated', async (_name, runner) => { + const lines = (await runner()(['api', 'user'])).split('\n'); + for (const [name, value] of Object.entries(expected)) expect(lines).toContain(`${name}=${value}`); + // The fixed settings are what ghEnvironment adds to an empty environment on this platform. + for (const [name, value] of Object.entries(ghEnvironment({}))) expect(lines).toContain(`${name}=${value}`); + expect(lines.some(line => line.startsWith('CODEBOOST_UNRELATED_SECRET='))).toBe(false); + }); +}); + +// The kill and pipe grace periods each gateway adds after its deadline, plus 500 ms for a slow test machine. +const settleBudgetMs: Record = { + merge: MERGE_KILL_GRACE_MS + MERGE_PIPE_GRACE_MS + 500, + issues: ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS + 500, +}; + +describe('gh stop waits', () => { + it('keep each deadline plus its stop wait below the 15-second serving request budget', () => { + // A merge click admitted just before shutdown must also settle inside the shutdown drain. + expect(MERGE_OPERATION_TIMEOUT_MS + MERGE_KILL_GRACE_MS + MERGE_PIPE_GRACE_MS).toBeLessThan(MAX_SHUTDOWN_DRAIN_MS); + expect(MAX_SHUTDOWN_DRAIN_MS).toBeLessThan(15_000); + // A merge-state or queue inspection, including the polls a browser makes. + expect(MERGE_INSPECTION_TIMEOUT_MS + MERGE_KILL_GRACE_MS + MERGE_PIPE_GRACE_MS).toBeLessThan(15_000); + expect(REFRESH_TIMEOUT_MS + ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS).toBeLessThan(15_000); + }); +}); + +describe('default gh runners stop a gh that ignores SIGTERM', () => { + let dir = ''; + const saved = process.env.PATH; + beforeAll(() => { + // A `gh` first on PATH that ignores SIGTERM and never exits on its own, and starts a process that keeps its output + // pipes open for 30 s, so the call can settle only through both grace periods. Once SIGTERM is ignored it writes + // both process IDs to `ready`, so the test does not abort before then (an early SIGTERM would still stop it). + dir = mkdtempSync(join(tmpdir(), 'codeboost-gh-stuck-')); + writeFileSync(join(dir, 'gh'), `#!/bin/sh\ntrap '' TERM\nsleep 30 &\necho $$ $! > '${join(dir, 'ready.tmp')}'\nmv '${join(dir, 'ready.tmp')}' '${join(dir, 'ready')}'\nwhile :; do sleep 1; done\n`); + chmodSync(join(dir, 'gh'), 0o755); + process.env.PATH = `${dir}:${saved ?? ''}`; + }); + // The fake gh's process ID, then the ID of the process that holds its pipes. + const readPids = () => readFileSync(join(dir, 'ready'), 'utf8').trim().split(' ').map(Number).filter(pid => pid > 0); + afterAll(() => { + if (saved === undefined) delete process.env.PATH; else process.env.PATH = saved; + rmSync(dir, { recursive: true, force: true }); + }); + + // A cancelled gh must not keep running after the caller was told the call stopped. + it.each(defaultRunners)('%s: an abort settles the call only after gh has exited', async (name, runner) => { + rmSync(join(dir, 'ready'), { force: true }); + const controller = new AbortController(); + let pids: number[] = []; + // Also on a failure or a test timeout: stop the call and the fake gh, so neither keeps running. + onTestFinished(() => { + controller.abort(new Error('test finished')); + if (!pids.length && existsSync(join(dir, 'ready'))) pids = readPids(); + for (const pid of pids) try { process.kill(pid, 'SIGKILL'); } catch { /* already exited */ } + }); + const call = runner()(['api', 'user'], { signal: controller.signal }); + const started = Date.now(); + while (!existsSync(join(dir, 'ready'))) { + if (Date.now() - started > 5_000) throw new Error('The fake gh did not start.'); + await new Promise(resolve => setTimeout(resolve, 10)); + } + pids = readPids(); + const aborted = Date.now(); + controller.abort(new Error('stop')); + await expect(call).rejects.toThrow('stop'); + expect(() => process.kill(pids[0]!, 0)).toThrow(expect.objectContaining({ code: 'ESRCH' })); + // The wait after an abort comes on top of the caller's deadline, so it must stay within the stated grace periods. + const budget = settleBudgetMs[name]; + if (budget !== undefined) expect(Date.now() - aborted).toBeLessThan(budget); + }, 15_000); +}); diff --git a/web/issues.ts b/web/issues.ts index 220bbd31..a26c3f75 100644 --- a/web/issues.ts +++ b/web/issues.ts @@ -15,7 +15,7 @@ function summarize(state: IssuePriorityState): IssueBoardState { } // Leaves headroom below the 15-second server request timeout for a request that joins an in-flight refresh. -const REFRESH_TIMEOUT_MS = 12_000; +export const REFRESH_TIMEOUT_MS = 12_000; /** * Server-owned issue list for the Issues screen. One refresh runs at a time; concurrent diff --git a/web/server.ts b/web/server.ts index 7382256e..feedccd9 100644 --- a/web/server.ts +++ b/web/server.ts @@ -5,7 +5,7 @@ import { randomBytes, timingSafeEqual } from 'node:crypto'; import { ReviewService, type ReviewConfig } from '../runner/review.ts'; import { Questions, type QuestionAgent } from '../runner/questions.ts'; import { GhMergeGateway, type MergeGateway } from '../github/merge.ts'; -import { MergeCoordinator, MergeNotApplied, MergeOutcomeUnknown } from '../runner/merge.ts'; +import { MERGE_OPERATION_TIMEOUT_MS, MergeCoordinator, MergeNotApplied, MergeOutcomeUnknown } from '../runner/merge.ts'; import { RunnerCoordinator, type RunnerDeps } from '../runner/coordinator.ts'; import { BadRequest, GuardRefusal, ShuttingDownError, assertUuidV4, isUuidV4, sameContext } from '../runner/lifecycle.ts'; import { GhIssueGateway, type IssueGateway } from '../github/issues.ts'; @@ -20,8 +20,10 @@ export interface PlanningDeps { describe(): Pick & { repo: { name: string; baseRef: string } }; } const publicRoot = new URL('./public/', import.meta.url); -export async function startServer(config: ReviewConfig, port = 4318, questionAgent?: QuestionAgent, mergeGateway?: MergeGateway, shutdownDrainMs = 14_500, issueGateway?: IssueGateway, runnerDeps?: RunnerDeps, planning?: PlanningDeps) { - if (!Number.isSafeInteger(shutdownDrainMs) || shutdownDrainMs < 1 || shutdownDrainMs > 14_500) throw new Error('Invalid shutdown drain deadline.'); +/** The longest shutdown waits for admitted requests to finish before aborting them; below the 15 s request timeout. */ +export const MAX_SHUTDOWN_DRAIN_MS = 14_500; +export async function startServer(config: ReviewConfig, port = 4318, questionAgent?: QuestionAgent, mergeGateway?: MergeGateway, shutdownDrainMs = MAX_SHUTDOWN_DRAIN_MS, issueGateway?: IssueGateway, runnerDeps?: RunnerDeps, planning?: PlanningDeps) { + if (!Number.isSafeInteger(shutdownDrainMs) || shutdownDrainMs < 1 || shutdownDrainMs > MAX_SHUTDOWN_DRAIN_MS) throw new Error('Invalid shutdown drain deadline.'); const service = new ReviewService(config), token = randomBytes(32).toString('hex'); let questions: Questions, merges: MergeCoordinator | null, issues: IssueBoard, runner: RunnerCoordinator | null, suggestions: SuggestionCoordinator | null; // Only coordinators' settlement and close code receive this; HTTP handlers never do. @@ -32,7 +34,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge // Issue retrieval is read-only, so demos may show it; they use a local fixture and never contact GitHub. issues = new IssueBoard(issueGateway ?? (config.demo ? demoIssueGateway() : config.github ? new GhIssueGateway(config.github.repository) : null), 'Issue ranking needs a GitHub repository. Add a github block with a repository to the review configuration.'); - merges = !config.demo && (mergeGateway || config.github) ? new MergeCoordinator(service, mergeGateway ?? new GhMergeGateway(config.github!), 14_000, capability) : null; + merges = !config.demo && (mergeGateway || config.github) ? new MergeCoordinator(service, mergeGateway ?? new GhMergeGateway(config.github!), MERGE_OPERATION_TIMEOUT_MS, capability) : null; // The runner starts only with an injected D; until #51 lands, runner actions report that it is unavailable. runner = runnerDeps ? new RunnerCoordinator(service.store, runnerDeps, undefined, capability) : null; // E3's settlement writes (completeSuggestions, settleSuggestion in close()) run with the shutdown capability.