test_an_unapproved_pr_is_refused makes a LIVE network call to real PR #42 on every presubmit run nf-yyx4 ← Beads

closed priority 2 task unassigned

Twinned from https://github.com/cfsh/ninefold/issues/760 by tools/beads/import_github.py's reconcile pass.

Filed unclaimed. Found while verifying the gh 2.45.0 → 2.97.0 upgrade (#742) — specifically while establishing whether the test suite could have caught a gh JSON-shape regression. It cannot, and this is the one exception, by accident.

The bug

PreconditionTest._Gh (tools/tests/test_merge_pr.py:257) subclasses the real Gh and overrides pr_view, unresolved_threads and merge — but not pr_files. Since 2026-08-06 the refusal path asks why Tier 1 did not carry the PR, which calls pr_files(42). That falls straight through to the real implementation:

``` gh api --paginate repos/cfsh/ninefold/pulls/42/files ```

Number 42 is not a fixture — it is the literal PR number _refuses() passes to M.merge(gh, 42), and cfsh/ninefold#42 is a real closed PR from this repo's history (*"chore(tools): whole-word rename-symbol.ps1 with dry-run"*, 2 files). So every ./presubmit.py run on every box quietly fetches that PR's file list over the network and feeds it to the perimeter check.

The test passes today only because those two paths — tools/README.md and tools/rename-symbol.ps1 — happen not to make PR #42 Tier-1 mergeable.

⚠️ The docstring asserts the opposite of what happens. It says *"this _Gh exposes no files and no verdict, so both of those reasons appear"*. It exposes files — real ones, over HTTP. The reason the assertion passes is not the reason the docstring gives.

Why it matters
Reproduction

Shadow gh with a stub that exits non-zero and run the lane:

```bash mkdir -p /tmp/mask && printf '#!/bin/sh\nexit 127\n' > /tmp/mask/gh && chmod +x /tmp/mask/gh PATH=/tmp/mask:$PATH python3 -m unittest discover -s tools/tests -p 'test_*.py' ```

``` ERROR: test_an_unapproved_pr_is_refused (test_merge_pr.PreconditionTest.test_an_unapproved_pr_is_refused) nflib.proc.ProcError: command failed (exit 127): gh api --paginate repos/cfsh/ninefold/pulls/42/files ```

Exactly one test fails that way — so this is also the measurement of how much live-gh coverage the suite has: one call, unintentional.

Suggested fix

Add pr_files to _Gh returning [] (or a path list chosen to express the case each test wants). That makes the docstring true — *"exposes no files"* — and makes the assertion pass for the stated reason rather than by coincidence.

⚠️ Worth checking the sibling stubs at the same time, rather than fixing this one site. _Gh subclasses the real Gh, so any method it does not override is a live call waiting for a code path to reach it; this one only appeared when the refusal path grew a pr_files call in August. The durable fix is for these stubs not to inherit a networking base class at all — nflib.fakes.FakeGh already exists and is what the rest of the suite uses. Whoever takes this should decide between the one-line patch and the conversion; I have not, deliberately, since it is a design call and not mine to make from here.

Not covered here

Nothing about the gh upgrade itself is blocked on this. #742's verification did not rely on the suite for compatibility evidence — it used a before/after structural fingerprint of the 23 read paths tools/ issues, precisely because the suite fakes the subprocess boundary everywhere else. See #742 and #759.

Refs #742.

Dependencies

None.

Comments

No comments.

Add a comment