fix(prs): lower default page size and surface real gh errors - #2912
fix(prs): lower default page size and surface real gh errors#2912santhiprakash wants to merge 2 commits into
Conversation
Busy repos hit GitHub GraphQL 504s when statusCheckRollup is fetched for 50 PRs at once; failures were misreported as missing gh auth. Default --limit to 20 with automatic smaller-page retries and propagate gh stderr. Fixes Graphify-Labs#2850
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Adds a --limit/-n flag to graphify prs and refactors gh fetching so fetch_prs retries with smaller page sizes (via new _page_limits) when a page fails. Replaces the opaque _gh return with _gh_call returning a (data, GhFailure) pair, surfacing real gh stderr and distinguishing missing-CLI from auth/network errors. Lowers the default PR page limit from 50 to 20 and adds tests covering _gh_call, _page_limits, and fetch_prs retry/error paths.
Worth a look
- Default PR fetch limit reduced from 50 to 20 —
graphify/prs.py:254· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- fetch_prs default limit reduced from 50 to 20 (silent behavior change) —
graphify/prs.py:258· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- int() on --limit argument can raise ValueError and crash cmd_prs —
graphify/prs.py:766· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 312 functions depend on the 130 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 119 callees - new:
fetch_prs()— 9 callers, 5 callees - new:
_build_server()— 2 callers, 16 callees - new:
cmd_prs()— 2 callers, 12 callees - new:
_status_color()— 4 callers, 5 callees - new:
_main()— 4 callers, 3 callees - new:
render_dashboard()— 1 callers, 10 callees - new:
render_pr_detail()— 1 callers, 8 callees - …and 5 more — each is listed as a finding
Verification — 312 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 144 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify cmd\_prs.
The verifier did not have enough to check cmd\_prs, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 9 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly AttributeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify fetch\_prs.
The verifier did not have enough to check fetch\_prs, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly RuntimeError — names the real obstacle, not a sampling gap)
No difference found (not proven): No behavior difference found in \_gh (not a proof).
The verifier ran both versions of \_gh on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 2 grounded finding(s) anchored inline below; 11 more finding(s) on lines outside this diff (see the check run).
| return _gh_call(*args) | ||
|
|
||
|
|
||
| def fetch_prs(repo: str | None = None, base: str | None = None, limit: int = _DEFAULT_PR_LIMIT) -> list[PRInfo]: |
There was a problem hiding this comment.
fetch_prs()
9 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| @@ -681,6 +738,7 @@ def triage_with_opus(prs: list[PRInfo], base: str) -> None: | |||
| def cmd_prs(argv: list[str]) -> None: | |||
There was a problem hiding this comment.
cmd_prs()
fans out to 12 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Reject non-numeric and non-positive --limit values with a clear CLI error instead of an uncaught ValueError traceback.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Adds a --limit/-n flag to graphify prs (default now 20, down from 50) with page-size auto-retry: fetch_prs walks _page_limits and falls back to smaller pages on failure. Refactors _gh into _gh_call returning a (data, GhFailure) tuple that distinguishes missing-CLI, timeout, non-zero exit, and JSON errors, and only appends the gh auth login hint when stderr actually looks auth-related. Adds _parse_limit input validation (exits 2 on non-integer/non-positive) and test coverage for the new failure classification, page-limit chains, and retry behavior.
Worth a look
- fetch_prs default limit changed from 50 to 20 —
graphify/prs.py:269· Escalate · medium · 2 independent checks- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- prs command default now fetches only 20 PRs —
graphify/prs.py:753· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 317 functions depend on the 135 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 119 callees - new:
fetch_prs()— 9 callers, 5 callees - new:
_build_server()— 2 callers, 16 callees - new:
cmd_prs()— 2 callers, 13 callees - new:
_status_color()— 4 callers, 5 callees - new:
_main()— 4 callers, 3 callees - new:
render_dashboard()— 1 callers, 10 callees - new:
render_pr_detail()— 1 callers, 8 callees - …and 5 more — each is listed as a finding
Verification — 317 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 149 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify cmd\_prs.
The verifier did not have enough to check cmd\_prs, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 9 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly AttributeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify fetch\_prs.
The verifier did not have enough to check fetch\_prs, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly RuntimeError — names the real obstacle, not a sampling gap)
No difference found (not proven): No behavior difference found in \_gh (not a proof).
The verifier ran both versions of \_gh on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 2 grounded finding(s) anchored inline below; 11 more finding(s) on lines outside this diff (see the check run).
| return _gh_call(*args) | ||
|
|
||
|
|
||
| def fetch_prs(repo: str | None = None, base: str | None = None, limit: int = _DEFAULT_PR_LIMIT) -> list[PRInfo]: |
There was a problem hiding this comment.
fetch_prs()
9 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| @@ -681,6 +750,7 @@ def triage_with_opus(prs: list[PRInfo], base: str) -> None: | |||
| def cmd_prs(argv: list[str]) -> None: | |||
There was a problem hiding this comment.
cmd_prs()
fans out to 13 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
1 participant
Problem
graphify prsrequestsstatusCheckRollupfor up to 50 open PRs in onegh pr listGraphQL call. On busy repos (e.g.cilium/cilium,apache/airflow) that page times out with HTTP 504 after six retries.When
ghfails,_ghreturnedNonefor every error class andfetch_prsalways raisedgh CLI not found or not authenticated, so a GraphQL timeout looked like an auth problem.Fixes #2850
Fix
--limit/-nongraphify prs._gh_callfrom_gh: preserveNonefor soft callers, but carryGhFailurewith real stderr/exit detail forfetch_prs.FileNotFoundErrordistinct (gh CLI not found…); only appendgh auth loginwhen stderr actually looks auth-related.Test
uv run pytest tests/test_prs.py -q— 56 passed