fix: close HTTP response in pipes() and fix 9 documentation issues#1
Merged
Conversation
Code fix: - pipes(): initialize response=None before try-block and add finally: response.close() to guarantee the connection is returned to the session pool in every code path (auth errors, JSON decode failures, unexpected exceptions) Documentation fixes: - Update test count from 234 to 249 across README, TESTING, CONTRIBUTING, and PULL_REQUEST_TEMPLATE (all missed or newly identified references) - Fix SECURITY.md Supported Versions table (malformed Markdown — rows were fused onto one line, breaking GitHub rendering) - Add SYNC_PROVIDER_ICONS valve to README Advanced configuration table - Correct HTTP 429 troubleshooting tip: MAX_RETRIES does not retry HTTP errors — it only retries on network timeouts and connection failures - Update bug_report.yml Pipe Version placeholder from 1.0.0 to 1.2.0 - Clarify TESTING.md §9.1 FALLBACK_MODELS test: make explicit that the primary model is the first element in the models array - Replace non-existent reasoning model example (claude-3.7-sonnet:thinking) with deepseek/deepseek-r1 in TESTING.md §3.2 - Add pipes() response-close fix entry to CHANGELOG [Unreleased] Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a potential HTTP connection leak in Pipe.pipes() by always closing the requests response object, and applies multiple documentation/template updates to reflect current test counts and clarify configuration/troubleshooting guidance.
Changes:
- Ensure the
/modelsHTTP response inpipes()is always closed via afinallyblock. - Update docs/templates to reflect the current unit test count (249) and improve clarity in testing/troubleshooting instructions.
- Fix Markdown rendering for the
SECURITY.mdsupported versions table.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
openrouter_pipe.py |
Always closes the /models HTTP response to prevent connection pool leaks on error/early-return paths. |
CHANGELOG.md |
Records the pipes() response-close fix under [Unreleased]. |
README.md |
Adds the SYNC_PROVIDER_ICONS valve row and corrects 429 retry guidance. |
SECURITY.md |
Repairs malformed Markdown table formatting for supported versions. |
TESTING.md |
Updates expected unit test count and clarifies fallback-model testing instructions. |
CONTRIBUTING.md |
Updates unit test count reference. |
.github/PULL_REQUEST_TEMPLATE.md |
Updates the checklist’s unit test count reference. |
.github/ISSUE_TEMPLATE/bug_report.yml |
Updates the pipe version placeholder example. |
.gitignore |
Adds ignore patterns for “Auto Claude” artifacts (scope expansion vs. PR description). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+317
to
+319
| finally: | ||
| if response is not None: | ||
| response.close() |
- Add 3 unit tests (15o/15p/15q) to verify pipes() always calls
response.close() via the finally block:
· 15o: auth-error path (HTTP 401) — close() called once
· 15p: JSON decode error path — close() called once
· 15q: success path — close() called once
This prevents future regressions of the connection-pool fix.
- Fix TESTING.md §0: replace "Must print 249/249 passed" with the
actual output format the runner emits ("All tests passed! ✓" and
"✗ Failed: 0"), addressing Copilot review comment.
- Update test count from 249 → 252 across README, CONTRIBUTING,
TESTING, and PULL_REQUEST_TEMPLATE to reflect the 3 new tests.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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.
Description
This PR delivers one code fix and nine documentation corrections identified during a comprehensive codebase audit.
Type of Change
Checklist
python test_pipe.py— 249/249 ✓)CHANGELOG.mdunder[Unreleased]Related Issues
N/A — identified during internal audit.
What Changed and Why
Code Fix —
pipes()HTTP response leakpipes()was callingself._session.get()but never explicitly closing the response object. In happy-path flowsresponse.json()drains the body and the connection is returned to the pool automatically, but if JSON decoding raised aValueError/JSONDecodeErrorthe socket could remain occupied until GC. Three affected code paths:401/403/502branch — readsresponse.json()then returns without closingexcept Exceptionhandler — entered whenresponse.json()on the success path failsexcept HTTPErrorhandler —exc.response(same object) not closedFix: initialise
response = Nonebefore thetryblock; addfinally: if response is not None: response.close(). This guarantees connection release regardless of which path exits.Documentation Fixes
PULL_REQUEST_TEMPLATE.md234/234— only file missed in previous update249/249SECURITY.mdREADME.md(Configuration)SYNC_PROVIDER_ICONSvalve missing from Advanced tableOPENROUTER_SYNC_ICONS, defaulttrueREADME.md(Troubleshooting)MAX_RETRIES— which does not retry HTTP errors, only network timeouts/connection failuresREADME.md(Project Structure)234249TESTING.md234/234(×2)249/249TESTING.md§3.2anthropic/claude-3.7-sonnet:thinkingdeepseek/deepseek-r1(matches integration tests)TESTING.md§9.1modelsarrayCONTRIBUTING.md234249bug_report.yml"e.g. 1.0.0"(initial release)"e.g. 1.2.0"CHANGELOG.mdpipes()response-close fix not recorded in[Unreleased]Reviewer Notes
response = Noneinit and a three-linefinallyblock). No logic changes.MAX_RETRIESdocumentation fix is important for correctness: the old text could lead users to incorrectly believe that raising the retry count would mitigate rate-limit errors.