Skip to content

Commit 608295e

Browse files
mchwangclaude
andcommitted
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>
1 parent 8b7ed5f commit 608295e

3 files changed

Lines changed: 25 additions & 13 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. 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.
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. 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: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,10 @@ 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.
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.
5657
- The gateway fetches open issues and the repository's current collaborators,
5758
excludes pull requests, follows bounded pagination for both collections, and
5859
validates every field used for normalization, trust, or ranking. If the

‎test/gh-env.test.ts‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,10 @@ import { GhMergeGateway, type RunGh } from '../github/merge.ts';
66
import { GhIssueGateway } from '../github/issues.ts';
77
import { GhPullRequestGateway } from '../github/pull-requests.ts';
88
import { GhAlreadyFixedGateway } from '../github/already-fixed.ts';
9+
import { GH_ENV_ALLOWLIST } from '../github/gh-env.ts';
910

1011
// Every adapter's own runner, not an injected one: these are the runners the server uses.
12+
// Each adapter must send every `gh` call through `run`, or this test does not see it.
1113
const defaultRunners: [string, () => RunGh][] = [
1214
['merge', () => new GhMergeGateway({ repository: 'owner/repo', pullRequest: 1, issue: 1 }).run],
1315
['issues', () => new GhIssueGateway('owner/repo').run],
@@ -17,26 +19,35 @@ const defaultRunners: [string, () => RunGh][] = [
1719

1820
describe('default gh runners', () => {
1921
let dir = '';
20-
let path: string | undefined;
22+
// A known value for every allowlisted variable, so the test can check that each one reaches gh.
23+
const expected: Record<string, string> = {};
24+
const saved = new Map<string, string | undefined>();
25+
const setEnv = (name: string, value: string | undefined) => {
26+
if (!saved.has(name)) saved.set(name, process.env[name]);
27+
if (value === undefined) delete process.env[name]; else process.env[name] = value;
28+
};
2129
beforeAll(() => {
2230
// A `gh` first on PATH that prints the environment it was given.
2331
dir = mkdtempSync(join(tmpdir(), 'codeboost-gh-env-'));
2432
writeFileSync(join(dir, 'gh'), '#!/bin/sh\nexec env\n');
2533
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';
34+
for (const name of GH_ENV_ALLOWLIST) {
35+
if (name === 'PATH') expected[name] = `${dir}:${process.env.PATH ?? ''}`;
36+
else if (name === 'LANG' || name === 'LC_ALL') expected[name] = 'C';
37+
else expected[name] = `sentinel-${name}`;
38+
setEnv(name, expected[name]);
39+
}
40+
setEnv('CODEBOOST_UNRELATED_SECRET', 'must-not-reach-gh');
2941
});
3042
afterAll(() => {
31-
process.env.PATH = path;
32-
delete process.env.CODEBOOST_UNRELATED_SECRET;
43+
for (const [name, value] of saved) if (value === undefined) delete process.env[name]; else process.env[name] = value;
3344
rmSync(dir, { recursive: true, force: true });
3445
});
3546

36-
it.each(defaultRunners)('%s: passes only the gh environment', async (_name, runner) => {
47+
it.each(defaultRunners)('%s: passes the allowlisted variables and nothing unrelated', async (_name, runner) => {
3748
const lines = (await runner()(['api', 'user'])).split('\n');
38-
expect(lines).toContain('GH_PROMPT_DISABLED=1');
39-
expect(lines).toContain(`PATH=${dir}:${path}`);
49+
for (const [name, value] of Object.entries(expected)) expect(lines).toContain(`${name}=${value}`);
50+
for (const setting of ['GH_PROMPT_DISABLED=1', 'GH_NO_UPDATE_NOTIFIER=1', 'GH_PAGER=cat', 'NO_COLOR=1']) expect(lines).toContain(setting);
4051
expect(lines.some(line => line.startsWith('CODEBOOST_UNRELATED_SECRET='))).toBe(false);
4152
});
4253
});

0 commit comments

Comments
 (0)