Skip to content

Commit 53a10de

Browse files
mchwangclaude
andcommitted
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>
1 parent 4baf097 commit 53a10de

5 files changed

Lines changed: 50 additions & 2 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 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.
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: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ 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 environment in
54+
`github/gh-env.ts`. Other variables from the server, such as unrelated
55+
credentials, are not passed on.
5356
- The gateway fetches open issues and the repository's current collaborators,
5457
excludes pull requests, follows bounded pagination for both collections, and
5558
validates every field used for normalization, trust, or ranking. If the

‎github/issues.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { execFile } from 'node:child_process';
22
import { promisify } from 'node:util';
3+
import { ghEnvironment } from './gh-env.ts';
34

45
const runFile = promisify(execFile);
56
const PAGE_SIZE = 100;
@@ -156,6 +157,7 @@ export class GhIssueGateway implements IssueGateway {
156157
this.run = run ?? (async (args, options) => (await runFile('gh', [...args], {
157158
maxBuffer: ISSUE_PAGE_MAX_BYTES,
158159
signal: options?.signal,
160+
env: ghEnvironment(),
159161
})).stdout);
160162
this.now = now;
161163
}

‎github/merge.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { execFile } from 'node:child_process';
22
import { promisify } from 'node:util';
3+
import { ghEnvironment } from './gh-env.ts';
34

45
const runFile = promisify(execFile);
56

@@ -87,7 +88,7 @@ export class GhMergeGateway implements MergeGateway, MergeQueueGateway {
8788
throw new Error('A GitHub repository, pull request, and issue are required for merging.');
8889
if (config.method !== undefined && !['merge','squash','rebase'].includes(config.method)) throw new Error('GitHub merge method must be merge, squash, or rebase.');
8990
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);
91+
this.run = run ?? (async (args, options) => (await runFile('gh', [...args], { timeout: 30_000, maxBuffer: 8 * 1024 * 1024, signal: options?.signal, env: ghEnvironment() })).stdout);
9192
}
9293

9394
async #json(args: readonly string[], signal?: AbortSignal): Promise<unknown> {

‎test/gh-env.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
2+
import { tmpdir } from 'node:os';
3+
import { join } from 'node:path';
4+
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
5+
import { GhMergeGateway, type RunGh } from '../github/merge.ts';
6+
import { GhIssueGateway } from '../github/issues.ts';
7+
import { GhPullRequestGateway } from '../github/pull-requests.ts';
8+
import { GhAlreadyFixedGateway } from '../github/already-fixed.ts';
9+
10+
// Every adapter's own runner, not an injected one: these are the runners the server uses.
11+
const defaultRunners: [string, () => RunGh][] = [
12+
['merge', () => new GhMergeGateway({ repository: 'owner/repo', pullRequest: 1, issue: 1 }).run],
13+
['issues', () => new GhIssueGateway('owner/repo').run],
14+
['pull requests', () => new GhPullRequestGateway({ repository: 'owner/repo' }).run],
15+
['already fixed', () => new GhAlreadyFixedGateway({ repository: 'owner/repo' }).run],
16+
];
17+
18+
describe('default gh runners', () => {
19+
let dir = '';
20+
let path: string | undefined;
21+
beforeAll(() => {
22+
// A `gh` first on PATH that prints the environment it was given.
23+
dir = mkdtempSync(join(tmpdir(), 'codeboost-gh-env-'));
24+
writeFileSync(join(dir, 'gh'), '#!/bin/sh\nexec env\n');
25+
chmodSync(join(dir, 'gh'), 0o755);
26+
path = process.env.PATH;
27+
process.env.PATH = `${dir}:${path}`;
28+
process.env.CODEBOOST_UNRELATED_SECRET = 'must-not-reach-gh';
29+
});
30+
afterAll(() => {
31+
process.env.PATH = path;
32+
delete process.env.CODEBOOST_UNRELATED_SECRET;
33+
rmSync(dir, { recursive: true, force: true });
34+
});
35+
36+
it.each(defaultRunners)('%s: passes only the gh environment', async (_name, runner) => {
37+
const lines = (await runner()(['api', 'user'])).split('\n');
38+
expect(lines).toContain('GH_PROMPT_DISABLED=1');
39+
expect(lines).toContain(`PATH=${dir}:${path}`);
40+
expect(lines.some(line => line.startsWith('CODEBOOST_UNRELATED_SECRET='))).toBe(false);
41+
});
42+
});

0 commit comments

Comments
 (0)