fix(ci): close HTTP error response bodies - #1879
Open
seonghobae wants to merge 3 commits into
Open
Conversation
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
seonghobae
enabled auto-merge (squash)
September 5, 2026 02:27
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
#1883) The admission-controller feature burst (#1859-#1869) shipped review_admission_controller.py, pr_review_merge_scheduler_core.py's SchedulerAdmissionGate, and (separately, pre-existing) a coverage gap in audit_codeql_default_setup_rollout.py without full test coverage or docstrings, breaking the required 100% coverage/docstring gate for every PR in this repository regardless of that PR's own diff. The original fix for this landed on .github#1871, which was later closed in favor of narrower successors (#1877 for the stale schedule oracles, #1879 for HTTP error response bodies) -- but the coverage and docstring portion of #1871's delta was dropped in that narrowing and never reached main. This PR recovers exactly that portion from #1871's still-present branch (fix/hourly-review-repair-callers-cron- format-drift) and completes it: - review_admission_controller.py: 85% -> 100% coverage (new tests/test_review_admission_controller.py), 14 missing docstrings added across its WorkerBoundary/AdmissionRequest/RequestRecord/ DispatchLease/ControllerState/DispatchPlan dataclasses and methods. - audit_codeql_default_setup_rollout.py: 79% -> 100% coverage (new tests/test_codeql_default_setup_rollout.py), 2 missing docstrings added (parse_args, main). - pr_review_merge_scheduler_core.py's SchedulerAdmissionGate: 3 missing docstrings added (__init__ and its two nested closures, lease/reconcile_state). Additionally closed pr_review_merge_scheduler_core.py's own separate, longer-standing coverage gap (98% -> 100%, unrelated to the admission-controller work) discovered while verifying this fix would actually bring main to a green gate rather than a differently-shaped 99%: the durable admission gate's own bounded-budget/stale-head branches across every dispatch call site (9 "admission_deferred" checks across post_update_branch_followup/dispatch_draft_review_only/ inspect_pr, plus dispatch_strix_evidence's own two "admission_deferred"/ "stale_head" pairs), reconcile()'s live-head-moved and still-running branches, rotating_pr_window's/dispatch_draft_review_only's/the workflow-run classifier's/the empty-PR-close path's/main()'s own --admission-state-path wiring's remaining gaps, and two untestable package-import fallback lines marked `# pragma: no cover - package import path` matching this file's established convention for that exact pattern. Full local triad: 2875 passed, 1 skipped; coverage 100%; interrogate 100%. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
…pline (#1909) Two rules from mistakes this session actually made and corrected, per the per-session lane split agreed with the other concurrent sessions (peer 3 took verification discipline in #1907; peer 2 has gate/merge mechanics; host 1 has close-time diff comparison and noema concurrency; host 2 has CI failure diagnosis). - Narrowing a PR does not carry its delta. #1871 was closed in favor of #1877 plus #1879; both successors were green, but neither carried the coverage/docstring delta, leaving main's required 100% gate broken until #1883 recovered it. "Each piece works" and "the pieces together cover the original's scope" are different questions. - Compare content, not ancestry. main mixes squash and merge commits (last 200: 153 single-parent, 47 two-parent, counted directly), so `git merge-base --is-ancestor` gives false negatives for squashed deltas and false positives for reverted merge-commit deltas. - Never endorse a timeout or retry constant on a model-invocation path without reading docs/product-goal-directive.md section 8, which accepts more than two hours per model and states speed is not a core consideration. #1889/#1890/#1892 each capped a model step at 900s on real multi-hour-hang evidence and were all reverted (#1891, #1895). Every PR number, the section-8 quotes, the parent-count distribution, and the 100% gate values were verified against the repository directly. An earlier draft of the timeout bullet cited a section number that does not exist and attributed a sentence to that file which appears only in #1891's PR body; both were caught by grepping rather than trusting the summary that introduced them, and that failure is recorded in the text. Full suite: 2883 passed, 1 skipped. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Noema LLM review
The PR correctly and safely closes HTTP error response bodies across four scripts using HTTPError.close(), which is idempotent and prevents resource leaks (file descriptors, connections). Each code change is accompanied by a regression test asserting the response body is closed. The changes are narrowly scoped, preserve existing behavior for non-HTTP errors, and introduce no correctness, security, or maintainability regressions. The only minor gap is that test_repository_metadata_live_verification.py does not cover the plain URLError (non-HTTP) branch, but that branch is unchanged by this PR, so it is non-blocking.
Reviewed changed lines
scripts/ci/noema_review_gate.py:1639 (RIGHT): Thefinallyblock guaranteesexc.close()is called even if_extract_http_error_telemetryraises. CPython'sHTTPError.close()is idempotent and closes the underlyingfp. The new testtest_call_llm_reports_only_safe_model_from_bounded_http_errorassertsresponse_body.closed, confirming the resource is released. The exception handling path is otherwise unchanged.scripts/ci/pingora_edge_policy.py:307 (RIGHT): Theisinstance(exc, HTTPError)check andexc.close()before re-raisingPolicyErrormatch CPython semantics. The testtest_github_open_json_raises_policy_error_for_transport_errorsassertsexc.fp.closed. Non-HTTPURLErrorandTimeoutErrorpaths are unaffected.scripts/ci/reconcile_repository_metadata.py:251 (RIGHT):HTTPErroris a subclass ofURLError, so theisinstanceguard correctly catches redirected 302 cases from_NoPagesRedirects. Theexc.close()call is idempotent; the new testtest_pages_transport_error_closes_response_bodyverifiesbody.closed. Non-HTTPURLError/timeout/OSError branches are untouched.scripts/ci/sandboxed_web_e2e.py:587 (RIGHT): Theexcept (urllib.error.URLError, TimeoutError) as excblock now checksisinstance(exc, urllib.error.HTTPError)and callsexc.close()before sleeping. This matches CPython'sHTTPError.close()semantics. The new testtest_wait_for_url_closes_http_error_responseassertsbody.closed. Retry and sleep logic are unchanged for other error types.tests/test_repository_metadata_live_verification.py:67 (RIGHT): The test constructs anHTTPErrorwith aBytesIObody and verifiesbody.closedafter_pages_publication_readyraises aRuntimeError. It exercises theHTTPErrorbranch of the newexc.close()logic. The non-HTTPURLErrorbranch is not covered here, but that branch was not modified by this PR.
Adversarial validation
scripts/ci/noema_review_gate.py:1639 (RIGHT)falsified: Callingexc.close()in afinallyblock could cause a double-close or an exception if_extract_http_error_telemetryalready closed the body. — CPython source (urllib/error.py L94-96) and the added testtest_call_llm_reports_only_safe_model_from_bounded_http_errorwhich assertsresponse_body.closedafter the call.scripts/ci/sandboxed_web_e2e.py:587 (RIGHT)falsified: Closing the HTTP error body in the polling loop could break the retry logic or cause a sleep to be skipped. — The test assertsbody.closedand thatwait_for_urlreturns False after the timeout period, proving the sleep path is unaffected.- Residual risk: Low. The only untested scenario is a plain (non-HTTP) URLError in reconcile_repository_metadata.py, but that code path is unchanged and existing tests cover transport errors broadly. The risk of a resource leak is mitigated by the idempotent close() and the confirmed tests for all HTTPError paths.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
0723a0c7d9d4da82e64f884cff8babf1f0e0c81a - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
git diff --checkpytest -q tests/test_noema_review_gate.py tests/test_pingora_edge_policy.py tests/test_repository_metadata_live_verification.py tests/test_sandboxed_web_e2e.py— 261 passedReplaces the only substantive runtime fixes from #1871 without its unrelated 928-line coverage/docstring expansion.