From 53a10dece0b0fb953a69eb0a3e19fdb9f89a49d2 Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 29 Sep 2026 12:51:23 -0700 Subject: [PATCH 1/9] Give the merge and issue gh runners the allowlisted environment GhMergeGateway and GhIssueGateway ran gh with the whole server environment. Their default runners now pass env: ghEnvironment(), the same as the pull request and already-fixed adapters. A new test runs each adapter's default runner against a fake gh that prints its environment. It checks that an unrelated variable does not reach gh. Before this change it failed for merge and issues. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- docs/implementation/issue-prioritization.md | 3 ++ github/issues.ts | 2 + github/merge.ts | 3 +- test/gh-env.test.ts | 42 +++++++++++++++++++++ 5 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 test/gh-env.test.ts diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index c16e77a4..44e5b421 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 environment in `github/gh-env.ts`; no other server variables are passed on. 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..1b21a2a2 100644 --- a/docs/implementation/issue-prioritization.md +++ b/docs/implementation/issue-prioritization.md @@ -50,6 +50,9 @@ 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 environment in + `github/gh-env.ts`. Other variables from the server, such as unrelated + credentials, are not passed on. - 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..731c3967 100644 --- a/github/issues.ts +++ b/github/issues.ts @@ -1,5 +1,6 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; +import { ghEnvironment } from './gh-env.ts'; const runFile = promisify(execFile); const PAGE_SIZE = 100; @@ -156,6 +157,7 @@ export class GhIssueGateway implements IssueGateway { this.run = run ?? (async (args, options) => (await runFile('gh', [...args], { maxBuffer: ISSUE_PAGE_MAX_BYTES, signal: options?.signal, + env: ghEnvironment(), })).stdout); this.now = now; } diff --git a/github/merge.ts b/github/merge.ts index 0a0596dd..a5d6fdb0 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,5 +1,6 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; +import { ghEnvironment } from './gh-env.ts'; const runFile = promisify(execFile); @@ -87,7 +88,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 ?? (async (args, options) => (await runFile('gh', [...args], { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal, env: ghEnvironment() })).stdout); } async #json(args: readonly string[], signal?: AbortSignal): Promise { diff --git a/test/gh-env.test.ts b/test/gh-env.test.ts new file mode 100644 index 00000000..9d3104f1 --- /dev/null +++ b/test/gh-env.test.ts @@ -0,0 +1,42 @@ +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { GhMergeGateway, type RunGh } from '../github/merge.ts'; +import { GhIssueGateway } from '../github/issues.ts'; +import { GhPullRequestGateway } from '../github/pull-requests.ts'; +import { GhAlreadyFixedGateway } from '../github/already-fixed.ts'; + +// Every adapter's own runner, not an injected one: these are the runners the server uses. +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 = ''; + let path: string | undefined; + 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); + path = process.env.PATH; + process.env.PATH = `${dir}:${path}`; + process.env.CODEBOOST_UNRELATED_SECRET = 'must-not-reach-gh'; + }); + afterAll(() => { + process.env.PATH = path; + delete process.env.CODEBOOST_UNRELATED_SECRET; + rmSync(dir, { recursive: true, force: true }); + }); + + it.each(defaultRunners)('%s: passes only the gh environment', async (_name, runner) => { + const lines = (await runner()(['api', 'user'])).split('\n'); + expect(lines).toContain('GH_PROMPT_DISABLED=1'); + expect(lines).toContain(`PATH=${dir}:${path}`); + expect(lines.some(line => line.startsWith('CODEBOOST_UNRELATED_SECRET='))).toBe(false); + }); +}); From 58aff56fa686ef2927aa2b62d08445c9df6d56a9 Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 29 Sep 2026 12:57:41 -0700 Subject: [PATCH 2/9] Check that every allowlisted gh variable reaches gh; fix doc wording Independent review findings: - The runner test now sets every GH_ENV_ALLOWLIST variable and checks each one and the fixed settings reach gh, so a runner that passes too little (and would break sign-in) fails too. - PATH and other changed variables are restored exactly, including when they were unset. - The docs now say gh also gets fixed settings that turn off prompts, the pager, colour and update checks. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- docs/implementation/issue-prioritization.md | 7 ++--- test/gh-env.test.ts | 29 ++++++++++++++------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index 44e5b421..bfebc74c 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. Each `gh` process gets only the allowlisted environment in `github/gh-env.ts`; no other server variables are passed on. 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. 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 1b21a2a2..ad20cb16 100644 --- a/docs/implementation/issue-prioritization.md +++ b/docs/implementation/issue-prioritization.md @@ -50,9 +50,10 @@ 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 environment in - `github/gh-env.ts`. Other variables from the server, such as unrelated - credentials, are not passed on. +- 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. - 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/test/gh-env.test.ts b/test/gh-env.test.ts index 9d3104f1..07b968c5 100644 --- a/test/gh-env.test.ts +++ b/test/gh-env.test.ts @@ -6,8 +6,10 @@ import { GhMergeGateway, type RunGh } from '../github/merge.ts'; import { GhIssueGateway } from '../github/issues.ts'; import { GhPullRequestGateway } from '../github/pull-requests.ts'; import { GhAlreadyFixedGateway } from '../github/already-fixed.ts'; +import { GH_ENV_ALLOWLIST } from '../github/gh-env.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], @@ -17,26 +19,35 @@ const defaultRunners: [string, () => RunGh][] = [ describe('default gh runners', () => { let dir = ''; - let path: string | undefined; + // 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); - path = process.env.PATH; - process.env.PATH = `${dir}:${path}`; - process.env.CODEBOOST_UNRELATED_SECRET = 'must-not-reach-gh'; + 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(() => { - process.env.PATH = path; - delete process.env.CODEBOOST_UNRELATED_SECRET; + 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 only the gh environment', async (_name, runner) => { + it.each(defaultRunners)('%s: passes the allowlisted variables and nothing unrelated', async (_name, runner) => { const lines = (await runner()(['api', 'user'])).split('\n'); - expect(lines).toContain('GH_PROMPT_DISABLED=1'); - expect(lines).toContain(`PATH=${dir}:${path}`); + for (const [name, value] of Object.entries(expected)) expect(lines).toContain(`${name}=${value}`); + for (const setting of ['GH_PROMPT_DISABLED=1', 'GH_NO_UPDATE_NOTIFIER=1', 'GH_PAGER=cat', 'NO_COLOR=1']) expect(lines).toContain(setting); expect(lines.some(line => line.startsWith('CODEBOOST_UNRELATED_SECRET='))).toBe(false); }); }); From d128cbc6bdbbe07a323537fb6acc775cff43ef10 Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 29 Sep 2026 17:58:58 -0700 Subject: [PATCH 3/9] Take the fixed gh settings from ghEnvironment in the runner test #84 now sets GH_PAGER per platform (empty on Windows, cat elsewhere). The test compares against ghEnvironment({}) instead of a hard-coded list, so it follows the helper's own values. Co-Authored-By: Claude Opus 5.5 --- test/gh-env.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/gh-env.test.ts b/test/gh-env.test.ts index 07b968c5..0b492773 100644 --- a/test/gh-env.test.ts +++ b/test/gh-env.test.ts @@ -6,7 +6,7 @@ import { GhMergeGateway, type RunGh } from '../github/merge.ts'; import { GhIssueGateway } from '../github/issues.ts'; import { GhPullRequestGateway } from '../github/pull-requests.ts'; import { GhAlreadyFixedGateway } from '../github/already-fixed.ts'; -import { GH_ENV_ALLOWLIST } from '../github/gh-env.ts'; +import { GH_ENV_ALLOWLIST, ghEnvironment } from '../github/gh-env.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. @@ -47,7 +47,8 @@ describe('default gh runners', () => { 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}`); - for (const setting of ['GH_PROMPT_DISABLED=1', 'GH_NO_UPDATE_NOTIFIER=1', 'GH_PAGER=cat', 'NO_COLOR=1']) expect(lines).toContain(setting); + // 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); }); }); From fd95baae3a2eb73eab09cacc1f89f23a5bfeb438 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 08:26:47 -0700 Subject: [PATCH 4/9] Stop the merge and issue gh runners before a cancelled call returns GhMergeGateway and GhIssueGateway ran gh through promisified execFile. On abort it sends SIGTERM and rejects at once, without waiting for gh to exit. A gh that ignores SIGTERM kept running, so a cancelled `gh pr merge` could still merge after the server reported it stopped. Both now use runWithInput, like the pull request and already-fixed adapters: SIGTERM, then SIGKILL after the grace period, and the promise settles only after the process has exited. A new test runs each default runner against a fake gh that ignores SIGTERM, aborts the call, and checks that gh has exited by the time the call rejects. It failed for merge and issues before this change. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- docs/implementation/issue-prioritization.md | 2 ++ github/issues.ts | 8 ++--- github/merge.ts | 7 ++--- test/gh-env.test.ts | 35 ++++++++++++++++++++- 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index bfebc74c..ae8b45f6 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. 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. 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 `gh` SIGTERM, then SIGKILL after five seconds, and the call returns only after the process has exited, so a cancelled `gh pr merge` cannot go on to merge. 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 ad20cb16..b4546043 100644 --- a/docs/implementation/issue-prioritization.md +++ b/docs/implementation/issue-prioritization.md @@ -54,6 +54,8 @@ dedicated read-only gateway with these boundaries: `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. +- A timeout or abort sends `gh` SIGTERM, then SIGKILL after five seconds. + The fetch returns only after the process has exited. - 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 731c3967..e61f0979 100644 --- a/github/issues.ts +++ b/github/issues.ts @@ -1,8 +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; @@ -154,11 +152,11 @@ 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, env: ghEnvironment(), - })).stdout); + })); this.now = now; } diff --git a/github/merge.ts b/github/merge.ts index a5d6fdb0..0b2ab382 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,8 +1,5 @@ -import { execFile } from 'node:child_process'; -import { promisify } from 'node:util'; import { ghEnvironment } from './gh-env.ts'; - -const runFile = promisify(execFile); +import { runWithInput } from './run-with-input.ts'; export interface RequiredCheck { context: string; @@ -88,7 +85,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, env: ghEnvironment() })).stdout); + this.run = run ?? ((args, options) => runWithInput('gh', args, { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal, env: ghEnvironment() })); } async #json(args: readonly string[], signal?: AbortSignal): Promise { diff --git a/test/gh-env.test.ts b/test/gh-env.test.ts index 0b492773..ac6446a1 100644 --- a/test/gh-env.test.ts +++ b/test/gh-env.test.ts @@ -1,4 +1,4 @@ -import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +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 } from 'vitest'; @@ -52,3 +52,36 @@ describe('default gh runners', () => { expect(lines.some(line => line.startsWith('CODEBOOST_UNRELATED_SECRET='))).toBe(false); }); }); + +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. Once SIGTERM is ignored it writes its + // process ID 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\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 ?? ''}`; + }); + afterAll(() => { + if (saved === undefined) delete process.env.PATH; else process.env.PATH = saved; + rmSync(dir, { recursive: true, force: true }); + }); + + // A cancelled `gh pr merge` must not go on to merge after the caller was told it 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(); + const call = runner()(['api', 'user'], { signal: controller.signal }); + while (!existsSync(join(dir, 'ready'))) await new Promise(resolve => setTimeout(resolve, 10)); + const pid = Number(readFileSync(join(dir, 'ready'), 'utf8')); + controller.abort(new Error('stop')); + try { + await expect(call).rejects.toThrow(); + expect(() => process.kill(pid, 0)).toThrow(expect.objectContaining({ code: 'ESRCH' })); + } finally { + try { process.kill(pid, 'SIGKILL'); } catch { /* already exited */ } + } + }, 15_000); +}); From 23bfb5a0e449852215833e7588bac21368fd860c Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 08:35:15 -0700 Subject: [PATCH 5/9] Count the gh stop wait inside the merge and issue deadlines Independent review round 1 on #86: - runWithInput's default 5 s kill grace and 1 s pipe grace let an aborted merge settle up to 20 s after it started, past the 15-second request budget (AGENTS.md: count shutdown grace inside the deadline). The merge and issue runners now use 0.5 s and 0.25 s: 14.75 s and 12.75 s. A test checks the settle time against these grace periods. - The merge doc said a cancelled gh pr merge cannot go on to merge. A request gh already sent can still be applied by GitHub; the doc now says such a merge is reported as unknown until it is reconciled. - The issue doc no longer mentions a per-command timeout the issue runner does not have. - The stuck-gh test now checks the abort reason and stops waiting for the fake gh after 5 s. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- docs/implementation/issue-prioritization.md | 7 ++++-- github/issues.ts | 8 ++++++ github/merge.ts | 9 ++++++- test/gh-env.test.ts | 28 +++++++++++++++------ 5 files changed, 43 insertions(+), 11 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index ae8b45f6..f8bbd6ae 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. 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 `gh` SIGTERM, then SIGKILL after five seconds, and the call returns only after the process has exited, so a cancelled `gh pr merge` cannot go on to merge. 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 half a second, and the call 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). Both waits count inside the 14-second merge deadline: 14.75 s stays below the 15-second request budget. A stopped `gh` cannot send anything more. A merge request it had already sent can still be applied by GitHub, so a cancelled merge is reported as unknown and its attempt stays in flight until it is reconciled with GitHub's state. 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 b4546043..7ad542a1 100644 --- a/docs/implementation/issue-prioritization.md +++ b/docs/implementation/issue-prioritization.md @@ -54,8 +54,11 @@ dedicated read-only gateway with these boundaries: `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. -- A timeout or abort sends `gh` SIGTERM, then SIGKILL after five seconds. - The fetch returns only after the process has exited. +- 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. Both waits count inside the 12-second + deadline: 12.75 s stays 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 e61f0979..3be420da 100644 --- a/github/issues.ts +++ b/github/issues.ts @@ -8,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 both count inside the 12-second fetch deadline, which stays + * below the 15-second serving request budget (12 + 0.5 + 0.25 = 12.75 s). + */ +export const ISSUE_KILL_GRACE_MS = 500, ISSUE_PIPE_GRACE_MS = 250; export type IssueAuthorAssociation = | 'OWNER' | 'MEMBER' | 'COLLABORATOR' | 'CONTRIBUTOR' @@ -156,6 +162,8 @@ export class GhIssueGateway implements IssueGateway { maxBuffer: ISSUE_PAGE_MAX_BYTES, signal: options?.signal, 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 0b2ab382..a9c03049 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -1,6 +1,13 @@ import { ghEnvironment } from './gh-env.ts'; import { runWithInput } from './run-with-input.ts'; +/** + * How long a stopped `gh` gets after SIGTERM before SIGKILL, and how long its inherited output pipes may stay open after + * it exits. A merge call settles only when gh has stopped, so both count inside the coordinator's 14-second operation + * deadline, which stays below the 15-second serving request budget (14 + 0.5 + 0.25 = 14.75 s). + */ +export const MERGE_KILL_GRACE_MS = 500, MERGE_PIPE_GRACE_MS = 250; + export interface RequiredCheck { context: string; appId: number | null; @@ -85,7 +92,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 ?? ((args, options) => runWithInput('gh', args, { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal, env: ghEnvironment() })); + 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 { diff --git a/test/gh-env.test.ts b/test/gh-env.test.ts index ac6446a1..c827c4b0 100644 --- a/test/gh-env.test.ts +++ b/test/gh-env.test.ts @@ -2,8 +2,8 @@ import { chmodSync, existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; -import { GhMergeGateway, type RunGh } from '../github/merge.ts'; -import { GhIssueGateway } from '../github/issues.ts'; +import { GhMergeGateway, 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'; @@ -53,6 +53,12 @@ describe('default gh runners', () => { }); }); +// The kill and pipe grace periods each gateway counts inside 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('default gh runners stop a gh that ignores SIGTERM', () => { let dir = ''; const saved = process.env.PATH; @@ -69,17 +75,25 @@ describe('default gh runners stop a gh that ignores SIGTERM', () => { rmSync(dir, { recursive: true, force: true }); }); - // A cancelled `gh pr merge` must not go on to merge after the caller was told it stopped. - it.each(defaultRunners)('%s: an abort settles the call only after gh has exited', async (_name, runner) => { + // 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(); const call = runner()(['api', 'user'], { signal: controller.signal }); - while (!existsSync(join(dir, 'ready'))) await new Promise(resolve => setTimeout(resolve, 10)); + 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)); + } const pid = Number(readFileSync(join(dir, 'ready'), 'utf8')); - controller.abort(new Error('stop')); try { - await expect(call).rejects.toThrow(); + const aborted = Date.now(); + controller.abort(new Error('stop')); + await expect(call).rejects.toThrow('stop'); expect(() => process.kill(pid, 0)).toThrow(expect.objectContaining({ code: 'ESRCH' })); + // The wait after an abort counts inside 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); } finally { try { process.kill(pid, 'SIGKILL'); } catch { /* already exited */ } } From 0b0f03042bac1bb6ec911746e97c07e16dbdd5df Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 08:44:37 -0700 Subject: [PATCH 6/9] Test the gh stop waits against the request budget Independent review round 2 on #86: - The stuck-gh test's grandchild released the pipes on its own after about 0.5 s, so dropping pipeGraceMs still passed. The fake gh now starts a process that holds the pipes for 30 s, so the call settles only through both grace periods, and the test kills it afterwards. - A test now checks that each deadline plus its stop wait stays below the 15-second request budget. The merge operation deadline and the issue refresh deadline are named constants the test imports. - Comments and docs say the stop wait comes on top of the deadline, that the merge constants cover every gh call of the gateway, and that an unknown direct merge stays disabled until GitHub shows it merged. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- docs/implementation/issue-prioritization.md | 4 +-- github/issues.ts | 4 +-- github/merge.ts | 5 ++-- runner/merge.ts | 6 +++-- test/gh-env.test.ts | 28 ++++++++++++++------- web/issues.ts | 2 +- web/server.ts | 4 +-- 8 files changed, 34 insertions(+), 21 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index f8bbd6ae..57f3348d 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. 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 half a second, and the call 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). Both waits count inside the 14-second merge deadline: 14.75 s stays below the 15-second request budget. A stopped `gh` cannot send anything more. A merge request it had already sent can still be applied by GitHub, so a cancelled merge is reported as unknown and its attempt stays in flight until it is reconciled with GitHub's state. 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 half a second, and the call 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 merge click aborted at its 14-second deadline settles by 14.75 s, below the 15-second request budget. A stopped `gh` cannot send anything more. A merge request it had already sent can still be applied by GitHub, so a cancelled merge is reported as unknown. Its attempt stays in flight, with the action disabled, until GitHub shows the pull request merged. 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 7ad542a1..99406702 100644 --- a/docs/implementation/issue-prioritization.md +++ b/docs/implementation/issue-prioritization.md @@ -57,8 +57,8 @@ dedicated read-only gateway with these boundaries: - 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. Both waits count inside the 12-second - deadline: 12.75 s stays below the 15-second request timeout. + 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 3be420da..b04d6059 100644 --- a/github/issues.ts +++ b/github/issues.ts @@ -10,8 +10,8 @@ const MAX_BODY_LENGTH = 65_536; 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 both count inside the 12-second fetch deadline, which stays - * below the 15-second serving request budget (12 + 0.5 + 0.25 = 12.75 s). + * 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; diff --git a/github/merge.ts b/github/merge.ts index a9c03049..5241ebde 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -3,8 +3,9 @@ import { runWithInput } from './run-with-input.ts'; /** * How long a stopped `gh` gets after SIGTERM before SIGKILL, and how long its inherited output pipes may stay open after - * it exits. A merge call settles only when gh has stopped, so both count inside the coordinator's 14-second operation - * deadline, which stays below the 15-second serving request budget (14 + 0.5 + 0.25 = 14.75 s). + * 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.75 s later. That keeps the merge click (14 s deadline in runner/merge.ts) at 14.75 s and the 12 s inspections at + * 12.75 s, below the 15-second serving request budget. */ export const MERGE_KILL_GRACE_MS = 500, MERGE_PIPE_GRACE_MS = 250; diff --git a/runner/merge.ts b/runner/merge.ts index f255fb12..c7f3bcf5 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -6,6 +6,8 @@ import { MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type M 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); } diff --git a/test/gh-env.test.ts b/test/gh-env.test.ts index c827c4b0..d70f602a 100644 --- a/test/gh-env.test.ts +++ b/test/gh-env.test.ts @@ -7,6 +7,8 @@ import { GhIssueGateway, ISSUE_KILL_GRACE_MS, ISSUE_PIPE_GRACE_MS } from '../git 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'; // 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. @@ -53,20 +55,28 @@ describe('default gh runners', () => { }); }); -// The kill and pipe grace periods each gateway counts inside its deadline, plus 500 ms for a slow test machine. +// The kill and pipe grace periods each gateway adds after its deadline, plus 400 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, + merge: MERGE_KILL_GRACE_MS + MERGE_PIPE_GRACE_MS + 400, + issues: ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS + 400, }; +describe('gh stop waits', () => { + it('keep each deadline plus its stop wait below the 15-second serving request budget', () => { + expect(MERGE_OPERATION_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. Once SIGTERM is ignored it writes its - // process ID to `ready`, so the test does not abort before then (an early SIGTERM would still stop it). + // 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\necho $$ > '${join(dir, 'ready.tmp')}'\nmv '${join(dir, 'ready.tmp')}' '${join(dir, 'ready')}'\nwhile :; do sleep 1; done\n`); + 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 ?? ''}`; }); @@ -85,17 +95,17 @@ describe('default gh runners stop a gh that ignores SIGTERM', () => { if (Date.now() - started > 5_000) throw new Error('The fake gh did not start.'); await new Promise(resolve => setTimeout(resolve, 10)); } - const pid = Number(readFileSync(join(dir, 'ready'), 'utf8')); + const [pid = 0, holder = 0] = readFileSync(join(dir, 'ready'), 'utf8').trim().split(' ').map(Number); try { const aborted = Date.now(); controller.abort(new Error('stop')); await expect(call).rejects.toThrow('stop'); expect(() => process.kill(pid, 0)).toThrow(expect.objectContaining({ code: 'ESRCH' })); - // The wait after an abort counts inside the caller's deadline, so it must stay within the stated grace periods. + // 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); } finally { - try { process.kill(pid, 'SIGKILL'); } catch { /* already exited */ } + for (const stray of [pid, holder]) try { if (stray > 0) process.kill(stray, 'SIGKILL'); } catch { /* already exited */ } } }, 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..76b75889 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'; @@ -32,7 +32,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. From c3c658cb3ae63e2264d3248acae89163b2e7c086 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 08:56:18 -0700 Subject: [PATCH 7/9] Keep a merge click's stop wait inside the shutdown drain Independent review round 3 on #86: - A merge click admitted just before shutdown could settle at 14.75 s, after the 14.5 s shutdown drain. The merge runner's grace periods are now 0.25 s and 0.15 s, so a click settles within 14.4 s. The drain limit is a named constant, and the budget test checks the merge click against it. - The stuck-gh test stops the call and the fake gh in onTestFinished, so a failure or test timeout leaves no process behind. - The merge doc says only a direct attempt waits for GitHub to show it merged; a queued attempt also settles on removal or failure. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- github/merge.ts | 6 ++-- test/gh-env.test.ts | 42 +++++++++++++++++----------- web/server.ts | 6 ++-- 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index 57f3348d..692088bd 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. 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 half a second, and the call 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 merge click aborted at its 14-second deadline settles by 14.75 s, below the 15-second request budget. A stopped `gh` cannot send anything more. A merge request it had already sent can still be applied by GitHub, so a cancelled merge is reported as unknown. Its attempt stays in flight, with the action disabled, until GitHub shows the pull request merged. 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. A stopped `gh` cannot send anything more. A merge request it had already sent can still be applied by GitHub, 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/github/merge.ts b/github/merge.ts index 5241ebde..235206a9 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -4,10 +4,10 @@ import { runWithInput } from './run-with-input.ts'; /** * 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.75 s later. That keeps the merge click (14 s deadline in runner/merge.ts) at 14.75 s and the 12 s inspections at - * 12.75 s, below the 15-second serving request budget. + * 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 the 12 s inspections within 12.4 s. */ -export const MERGE_KILL_GRACE_MS = 500, MERGE_PIPE_GRACE_MS = 250; +export const MERGE_KILL_GRACE_MS = 250, MERGE_PIPE_GRACE_MS = 150; export interface RequiredCheck { context: string; diff --git a/test/gh-env.test.ts b/test/gh-env.test.ts index d70f602a..fbfb7884 100644 --- a/test/gh-env.test.ts +++ b/test/gh-env.test.ts @@ -1,7 +1,7 @@ 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 } from 'vitest'; +import { afterAll, beforeAll, describe, expect, it, onTestFinished } from 'vitest'; import { GhMergeGateway, 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'; @@ -9,6 +9,7 @@ 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. @@ -55,15 +56,17 @@ describe('default gh runners', () => { }); }); -// The kill and pipe grace periods each gateway adds after its deadline, plus 400 ms for a slow test machine. +// 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 + 400, - issues: ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS + 400, + 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', () => { - expect(MERGE_OPERATION_TIMEOUT_MS + MERGE_KILL_GRACE_MS + MERGE_PIPE_GRACE_MS).toBeLessThan(15_000); + // 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); expect(REFRESH_TIMEOUT_MS + ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS).toBeLessThan(15_000); }); }); @@ -80,6 +83,8 @@ describe('default gh runners stop a gh that ignores SIGTERM', () => { 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 }); @@ -89,23 +94,26 @@ describe('default gh runners stop a gh that ignores SIGTERM', () => { 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)); } - const [pid = 0, holder = 0] = readFileSync(join(dir, 'ready'), 'utf8').trim().split(' ').map(Number); - try { - const aborted = Date.now(); - controller.abort(new Error('stop')); - await expect(call).rejects.toThrow('stop'); - expect(() => process.kill(pid, 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); - } finally { - for (const stray of [pid, holder]) try { if (stray > 0) process.kill(stray, 'SIGKILL'); } catch { /* already exited */ } - } + 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/server.ts b/web/server.ts index 76b75889..feedccd9 100644 --- a/web/server.ts +++ b/web/server.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. From 249f04f04af8c3ae6ec3acab6c2842e3090f6b83 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 09:04:59 -0700 Subject: [PATCH 8/9] Name the merge inspection deadline and test it against the budget Independent review round 4 on #86: - The 12 s inspection deadline was a literal in five places, so the comment's "inspections settle within 12.4 s" had no test. It is now MERGE_INSPECTION_TIMEOUT_MS, the default and the cap of every merge inspection, and the budget test checks it plus the stop wait. - The merge doc no longer says a stopped gh cannot send anything more; it says stopping gh cannot recall a request it already sent. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/guarded-merge.md | 2 +- github/merge.ts | 14 ++++++++------ runner/merge.ts | 4 ++-- test/gh-env.test.ts | 4 +++- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/docs/implementation/guarded-merge.md b/docs/implementation/guarded-merge.md index 692088bd..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. 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. A stopped `gh` cannot send anything more. A merge request it had already sent can still be applied by GitHub, 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. +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/github/merge.ts b/github/merge.ts index 235206a9..aa0ac3d5 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -5,9 +5,11 @@ import { runWithInput } from './run-with-input.ts'; * 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 the 12 s inspections within 12.4 s. + * 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 default and the longest deadline of one inspection (merge state, queue watermark, queue state). */ +export const MERGE_INSPECTION_TIMEOUT_MS = 12_000; export interface RequiredCheck { context: string; @@ -246,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; @@ -269,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(); @@ -304,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 c7f3bcf5..a27b8858 100644 --- a/runner/merge.ts +++ b/runner/merge.ts @@ -1,7 +1,7 @@ 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; @@ -363,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 index fbfb7884..5d58b173 100644 --- a/test/gh-env.test.ts +++ b/test/gh-env.test.ts @@ -2,7 +2,7 @@ import { chmodSync, existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterAll, beforeAll, describe, expect, it, onTestFinished } from 'vitest'; -import { GhMergeGateway, MERGE_KILL_GRACE_MS, MERGE_PIPE_GRACE_MS, type RunGh } from '../github/merge.ts'; +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'; @@ -67,6 +67,8 @@ describe('gh stop waits', () => { // 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); }); }); From 0840dc43dcaa3eb2ff8e1918e019d779e8e9d7fc Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 09:10:13 -0700 Subject: [PATCH 9/9] Say which inspections default to the 12-second deadline The queue watermark defaults to 6 s; 12 s is only its cap. Co-Authored-By: Claude Opus 5.5 --- github/merge.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/merge.ts b/github/merge.ts index aa0ac3d5..81b5d58c 100644 --- a/github/merge.ts +++ b/github/merge.ts @@ -8,7 +8,7 @@ import { runWithInput } from './run-with-input.ts'; * 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 default and the longest deadline of one inspection (merge state, queue watermark, queue state). */ +/** 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 {