Skip to content

Give the merge and issue gh runners the allowlisted environment - #86

Merged
mchwang merged 9 commits into
mainfrom
feat/gh-env-merge-issues
Oct 1, 2026
Merged

Give the merge and issue gh runners the allowlisted environment#86
mchwang merged 9 commits into
mainfrom
feat/gh-env-merge-issues

Conversation

@mchwang

@mchwang mchwang commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Stacked on #84. Merge #84 first. After that, this PR's base changes to main.

What this changes

The merge adapter (GhMergeGateway) and the issue adapter (GhIssueGateway) ran gh with the whole server environment, including unrelated credentials. Their default runners now pass env: ghEnvironment() from github/gh-env.ts (added in #84).

With this change, every place that runs gh uses the allowlisted environment:

Adapter File Status
Merge github/merge.ts Changed in this PR
Issues github/issues.ts Changed in this PR
Pull requests github/pull-requests.ts (through github/run-with-input.ts) Already done in #84
Already fixed github/already-fixed.ts (through github/run-with-input.ts) Already done in #84

Test

test/gh-env.test.ts runs each adapter's own default runner against a fake gh that prints its environment. For each adapter it checks:

  • every allowlisted variable reaches gh
  • the fixed settings reach gh: GH_PROMPT_DISABLED, GH_NO_UPDATE_NOTIFIER, GH_PAGER, NO_COLOR. The test takes their values from ghEnvironment({}), so it follows the platform-specific GH_PAGER added in F2d: already-fixed check and opening the task's PR #84.
  • an unrelated variable does not reach gh

Evidence that the test catches failures:

  • With the original merge.ts and issues.ts, the merge and issue cases fail.
  • With a merge runner that passes only PATH, the merge case fails.

Docs

docs/implementation/guarded-merge.md and docs/implementation/issue-prioritization.md now say what environment gh gets.

Validation

These results are for head 1aebc6d, rebased onto #84 at 6c15c41:

  • npm run typecheck: passes.
  • CI's non-Docker test command: 44 files and 899 tests pass.
  • The merge, issues, publish, already-fixed and gh environment test files: 273 tests pass. The pull-request and already-fixed cases test F2d: already-fixed check and opening the task's PR #84's runWithInput runner.
  • This PR's CI: pending.

Earlier heads:

main's #83 (history Git with an allowlisted environment) changes only git/history.ts and its test, so it does not overlap with this PR.

Independent review

A separate review agent, which did not share the author's context, reviewed this PR in 2 rounds.

Round 1 found no bugs. It confirmed that no other code runs gh. It also confirmed that the allowlist covers what gh pr merge --repo and gh api need: sign-in through tokens, the config directory and the keyring, plus proxy and CA settings.

Round 1 findings:

Finding Result Rule
The test did not check that allowlisted variables, including sign-in, reach gh. Fixed: the test now checks every allowlisted variable. Existing rule: AGENTS.md "Give every subprocess an explicit allowlisted environment … and test that each is passed".
The docs said "only the allowlisted environment", but gh also gets the fixed settings. Fixed. Existing rule: AGENTS.md "Treat every behavioural claim … as something to verify".
The test did not restore PATH correctly when it was unset. Fixed: every changed variable is restored exactly. One-off test-hygiene fix.
The test cannot see a gh call that does not go through run. Fixed: a comment now says every gh call must go through run. One-off.
Move the ghEnvironment() unit tests out of publish.test.ts. Declined: those tests belong to #84's files. One-off.
The test fails on a host where /tmp cannot run programs. Declined: CI runs on Ubuntu, and existing tests use the same setup. One-off.
DISPLAY and Windows variables are not allowlisted. Declined: they concern #84's allowlist, not this change. One-off.

Round 2 reread the full diff and found nothing new. It checked that the test's environment changes cannot leak into other test files and are restored if setup fails partway.

No AGENTS.md changes are needed: each finding is covered by an existing rule or is a one-off.

🤖 Generated with Claude Code

@mchwang
mchwang requested a balanced review from Copilot September 29, 2026 20:42
Copilot AI reviewed Sep 29, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation consistently applies the shared environment policy and includes focused regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

This PR restricts merge and issue GitHub CLI subprocesses to the shared allowlisted environment.

Changes:

  • Applies ghEnvironment() to merge and issue runners.
  • Adds regression coverage for all four GitHub adapters.
  • Documents subprocess environment isolation.
File Description
github/​merge.ts Applies the allowlisted environment to merge commands.
github/​issues.ts Applies the allowlisted environment to issue commands.
test/​gh-env.test.ts Verifies allowed and unrelated environment variables.
docs/​implementation/​guarded-merge.md Documents merge subprocess isolation.
docs/​implementation/​issue-prioritization.md Documents issue subprocess isolation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@mchwang
mchwang force-pushed the feat/gh-env-merge-issues branch from 7019d51 to b33eb2a Compare September 29, 2026 21:09
@mchwang mchwang mentioned this pull request Sep 29, 2026
Merged
@mchwang
mchwang force-pushed the feat/gh-env-merge-issues branch 12 times, most recently from 55da8fb to 1aebc6d Compare September 30, 2026 06:56
mchwang and others added 3 commits October 1, 2026 00:52
@mchwang @claude
Give the merge and issue gh runners the allowlisted environment
53a10de 
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>
@mchwang @claude
Check that every allowlisted gh variable reaches gh; fix doc wording
58aff56 
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>
@mchwang @claude
Take the fixed gh settings from ghEnvironment in the runner test
d128cbc 
#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>
@mchwang
mchwang force-pushed the feat/gh-env-merge-issues branch from 1aebc6d to d128cbc Compare October 1, 2026 08:03
@mchwang
mchwang changed the base branch from feat/f2d-already-fixed-pr to main October 1, 2026 08:03
mchwang and others added 6 commits October 1, 2026 08:26
@mchwang @claude
Stop the merge and issue gh runners before a cancelled call returns
fd95baa 
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>
@mchwang @claude
Count the gh stop wait inside the merge and issue deadlines
23bfb5a 
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>
@mchwang @claude
Test the gh stop waits against the request budget
0b0f030 
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>
@mchwang @claude
Keep a merge click's stop wait inside the shutdown drain
c3c658c 
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>
@mchwang @claude
Name the merge inspection deadline and test it against the budget
249f04f 
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>
@mchwang @claude
Say which inspections default to the 12-second deadline
0840dc4 
The queue watermark defaults to 6 s; 12 s is only its cap.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mchwang
mchwang marked this pull request as ready for review October 1, 2026 16:45
@mchwang
mchwang merged commit aafd153 into main Oct 1, 2026
1 check passed
@mchwang mchwang mentioned this pull request Oct 1, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mchwang