Skip to content

Commit aafd153

Browse files
mchwangclaude
andauthored
Give the merge and issue gh runners the allowlisted environment (#86)
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 4baf097 commit aafd153

8 files changed

Lines changed: 174 additions & 24 deletions

File tree

‎docs/implementation/guarded-merge.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ Merging blocks when any plan item is unreviewed or stale, an attributed file is
1010

1111
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.
1212

13-
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.
13+
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.
1414

1515
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.
1616

‎docs/implementation/issue-prioritization.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,15 @@ dedicated read-only gateway with these boundaries:
5050

5151
- Codeboost invokes `gh` with literal arguments; issue text is parsed only as
5252
data and is never interpolated into a shell command or prompt.
53+
- Each `gh` process gets only the allowlisted variables in
54+
`github/gh-env.ts`, plus fixed settings that turn off prompts, the pager,
55+
colour and update checks. Other variables from the server, such as
56+
unrelated credentials, are not passed on.
57+
- When the fetch deadline passes or the fetch is cancelled, the `gh` process
58+
gets SIGTERM, then SIGKILL after half a second. The fetch returns only after
59+
that process has exited, or a quarter of a second after it exits if a process
60+
it started keeps its output open. So a fetch aborted at its 12-second
61+
deadline settles by 12.75 s, below the 15-second request timeout.
5362
- The gateway fetches open issues and the repository's current collaborators,
5463
excludes pull requests, follows bounded pagination for both collections, and
5564
validates every field used for normalization, trust, or ranking. If the

‎github/issues.ts‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,19 @@
1-
import { execFile } from 'node:child_process';
2-
import { promisify } from 'node:util';
1+
import { ghEnvironment } from './gh-env.ts';
2+
import { runWithInput } from './run-with-input.ts';
33

4-
const runFile = promisify(execFile);
54
const PAGE_SIZE = 100;
65
const MAX_PAGES = 10;
76
const MAX_ISSUES = PAGE_SIZE * MAX_PAGES;
87
const MAX_COLLABORATORS = PAGE_SIZE * MAX_PAGES;
98
const MAX_BODY_LENGTH = 65_536;
109
// Covers one bounded 100-record page, including JSON-escaped bodies, labels and response overhead.
1110
export const ISSUE_PAGE_MAX_BYTES = 64 * 1024 * 1024;
11+
/**
12+
* How long a stopped `gh` gets after SIGTERM before SIGKILL, and how long its inherited output pipes may stay open after
13+
* it exits. A fetch settles only when gh has stopped, so a fetch aborted at its 12 s deadline (web/issues.ts) settles up
14+
* to 0.75 s later: 12.75 s, below the 15-second serving request budget.
15+
*/
16+
export const ISSUE_KILL_GRACE_MS = 500, ISSUE_PIPE_GRACE_MS = 250;
1217

1318
export type IssueAuthorAssociation =
1419
| 'OWNER' | 'MEMBER' | 'COLLABORATOR' | 'CONTRIBUTOR'
@@ -153,10 +158,13 @@ export class GhIssueGateway implements IssueGateway {
153158
constructor(repository: string, run?: RunGh, now: () => Date = () => new Date()) {
154159
if (!repositoryName(repository)) throw new Error('A GitHub repository is required for issue retrieval.');
155160
this.repository = repository;
156-
this.run = run ?? (async (args, options) => (await runFile('gh', [...args], {
161+
this.run = run ?? ((args, options) => runWithInput('gh', args, {
157162
maxBuffer: ISSUE_PAGE_MAX_BYTES,
158163
signal: options?.signal,
159-
})).stdout);
164+
env: ghEnvironment(),
165+
killGraceMs: ISSUE_KILL_GRACE_MS,
166+
pipeGraceMs: ISSUE_PIPE_GRACE_MS,
167+
}));
160168
this.now = now;
161169
}
162170

‎github/merge.ts‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,15 @@
1-
import { execFile } from 'node:child_process';
2-
import { promisify } from 'node:util';
1+
import { ghEnvironment } from './gh-env.ts';
2+
import { runWithInput } from './run-with-input.ts';
33

4-
const runFile = promisify(execFile);
4+
/**
5+
* How long a stopped `gh` gets after SIGTERM before SIGKILL, and how long its inherited output pipes may stay open after
6+
* it exits. Every gh call of this gateway settles only when gh has stopped, so a call aborted at a deadline settles up
7+
* 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
8+
* drain and below the 15-second serving request budget, and an inspection (at most 12 s) within 12.4 s.
9+
*/
10+
export const MERGE_KILL_GRACE_MS = 250, MERGE_PIPE_GRACE_MS = 150;
11+
/** The longest deadline of any inspection, and the default for merge-state and queue-state inspections. */
12+
export const MERGE_INSPECTION_TIMEOUT_MS = 12_000;
513

614
export interface RequiredCheck {
715
context: string;
@@ -87,7 +95,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway {
8795
throw new Error('A GitHub repository, pull request, and issue are required for merging.');
8896
if (config.method !== undefined && !['merge','squash','rebase'].includes(config.method)) throw new Error('GitHub merge method must be merge, squash, or rebase.');
8997
this.config = config;
90-
this.run = run ?? (async (args, options) => (await runFile('gh', [...args], { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal })).stdout);
98+
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 }));
9199
}
92100

93101
async #json(args: readonly string[], signal?: AbortSignal): Promise<unknown> {
@@ -240,8 +248,8 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway {
240248
}
241249

242250
async inspect(options: { fresh?: boolean; timeoutMs?: number; signal?: AbortSignal } = {}): Promise<RemoteMergeState> {
243-
const timeoutMs = options.timeoutMs ?? 12_000;
244-
if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > 12_000) throw new Error('Invalid GitHub inspection timeout.');
251+
const timeoutMs = options.timeoutMs ?? MERGE_INSPECTION_TIMEOUT_MS;
252+
if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > MERGE_INSPECTION_TIMEOUT_MS) throw new Error('Invalid GitHub inspection timeout.');
245253
if (!options.fresh && this.#cache && this.#cache.expiresAt > Date.now()) return this.#cache.state;
246254
const generation = this.#generation;
247255
if (!options.fresh && this.#inflight?.generation === generation) return this.#inflight.promise;
@@ -263,7 +271,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway {
263271
async queueWatermark(expectedHead: string, options: { signal?: AbortSignal; timeoutMs?: number } = {}): Promise<string | null> {
264272
fullSha(expectedHead, 'expected head SHA');
265273
const timeoutMs = options.timeoutMs ?? 6_000;
266-
if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > 12_000) throw new Error('Invalid GitHub queue watermark timeout.');
274+
if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > MERGE_INSPECTION_TIMEOUT_MS) throw new Error('Invalid GitHub queue watermark timeout.');
267275
const [owner, name] = this.config.repository.split('/') as [string, string];
268276
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}}}}}}`;
269277
const timeout = new AbortController();
@@ -298,8 +306,8 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway {
298306

299307
async inspectQueue(expectedHead: string, options: { signal?: AbortSignal; timeoutMs?: number; afterCursor?: string | null } = {}): Promise<MergeQueueObservation> {
300308
fullSha(expectedHead, 'expected head SHA');
301-
const timeoutMs = options.timeoutMs ?? 12_000;
302-
if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > 12_000) throw new Error('Invalid GitHub queue inspection timeout.');
309+
const timeoutMs = options.timeoutMs ?? MERGE_INSPECTION_TIMEOUT_MS;
310+
if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > MERGE_INSPECTION_TIMEOUT_MS) throw new Error('Invalid GitHub queue inspection timeout.');
303311
const correlated = Object.hasOwn(options, 'afterCursor');
304312
if (options.afterCursor !== undefined && options.afterCursor !== null && (typeof options.afterCursor !== 'string' || !options.afterCursor || options.afterCursor.length > 512))
305313
throw new Error('Invalid merge-queue event cursor.');

‎runner/merge.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
import type { ReviewService } from './review.ts';
22
import { mergeActionResponse, type MergeAttempt } from './store.ts';
33
import { ActionIdReused, GuardRefusal, MERGEABLE_STATUSES, ShuttingDownError, assertUuidV4, settleWith, type ShutdownCapability } from './lifecycle.ts';
4-
import { MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type MergeResult, type RemoteMergeState } from '../github/merge.ts';
4+
import { MERGE_INSPECTION_TIMEOUT_MS, MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type MergeResult, type RemoteMergeState } from '../github/merge.ts';
55

66
type ReviewView = ReturnType<ReviewService['load']>;
77
type QueueGateway = MergeGateway & MergeQueueGateway;
88
export interface MergeBlocker { code: string; message: string; }
9+
/** The longest a merge click may run before it is aborted; its last gh call's stop wait comes on top (github/merge.ts). */
10+
export const MERGE_OPERATION_TIMEOUT_MS = 14_000;
911
const storageError = (error: unknown) => (error as { code?: string } | null)?.code === 'ERR_SQLITE_ERROR';
1012
/** The merge was not applied for a passing reason (deadline, shutdown); the same click may be sent again. */
1113
export class MergeNotApplied extends Error {}
@@ -44,8 +46,8 @@ export class MergeCoordinator {
4446
readonly operationTimeoutMs: number;
4547
/** Settlement of an irreversible merge keeps its writes after the Store gate closes; request-path reconciliation does not. */
4648
#settle: <T>(fn: () => T) => T;
47-
constructor(service: ReviewService, gateway: MergeGateway, operationTimeoutMs = 14_000, capability?: ShutdownCapability) {
48-
if (!Number.isSafeInteger(operationTimeoutMs) || operationTimeoutMs < 1 || operationTimeoutMs > 14_000) throw new Error('Invalid merge operation deadline.');
49+
constructor(service: ReviewService, gateway: MergeGateway, operationTimeoutMs = MERGE_OPERATION_TIMEOUT_MS, capability?: ShutdownCapability) {
50+
if (!Number.isSafeInteger(operationTimeoutMs) || operationTimeoutMs < 1 || operationTimeoutMs > MERGE_OPERATION_TIMEOUT_MS) throw new Error('Invalid merge operation deadline.');
4951
this.service = service; this.gateway = gateway; this.operationTimeoutMs = operationTimeoutMs;
5052
this.#settle = settleWith(capability);
5153
}
@@ -361,7 +363,7 @@ export class MergeCoordinator {
361363

362364
async #pollQueue(attempt: MergeAttempt, signal: AbortSignal): Promise<MergeQueueStatus | null> {
363365
try {
364-
const observation = await (this.gateway as QueueGateway).inspectQueue(attempt.reviewedHead, { signal, timeoutMs: 12_000, afterCursor: attempt.queueWatermark ?? null });
366+
const observation = await (this.gateway as QueueGateway).inspectQueue(attempt.reviewedHead, { signal, timeoutMs: MERGE_INSPECTION_TIMEOUT_MS, afterCursor: attempt.queueWatermark ?? null });
365367
this.#publishQueueObservation(attempt, observation);
366368
return this.#queueStatus();
367369
} catch (error) {

0 commit comments

Comments
 (0)