fix(egress): bound provider error-body buffering in _open_provider (#1041) - #1133
Conversation
_open_provider read an HTTP >=400 error body with an unbounded response.read() before wrapping it in urllib.error.HTTPError. The provider-failure taxonomy only ever inspects MAX_PROVIDER_ERROR_BODY_BYTES of that body, so a hostile or broken provider could stream unbounded data on a request that is rejected anyway. Cap the read at MAX_PROVIDER_ERROR_BODY_BYTES + 1 and slice to the cap, mirroring provider_error_body(). Status, reason, and headers are preserved so 401/429/503 classification is unchanged. Refs #1041
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughProvider 오류 응답 본문을 ChangesProvider 오류 응답 처리
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to Provider error responses are now bounded without changing status or retry behavior, and no actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 1 files. (1 skipped: 1 too large.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
contextual_orchestrator/orchestrator.py— Python module behaviortests/test_open_provider_error_body_bounds.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Python: orchestrator.py"]
S1 --> I1["Python module behavior"]
I1 --> R1["Review risk: Python: orchestrator.py"]
R1 --> V1["pytest plus coverage"]
Evidence --> S2["Test: test_open_provider_error_body_bounds.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_open_provider_error_body_bounds.py"]
R2 --> V2["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
d222dced2c6f578c75ffc8938bf4800cc510f3c9 - Workflow run: 34554149064
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Python: orchestrator.py"]
S1 --> I1["Python module behavior"]
I1 --> R1["Review risk: Python: orchestrator.py"]
R1 --> V1["pytest plus coverage"]
Evidence --> S2["Test: test_open_provider_error_body_bounds.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_open_provider_error_body_bounds.py"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The change bounds the upstream error-body buffering in _open_provider to MAX_PROVIDER_ERROR_BODY_BYTES bytes by reading one extra byte and slicing, while preserving local status, reason, and headers before wrapping the response. The new test module directly exercises the bounded read size, truncation behavior, classification impact, and in-bounds JSON integrity. Import of the constant is verified by successful test import. With the reviewed probes falsifying concrete regression hypotheses, no blocking issues were found.
Reviewed changed lines
contextual_orchestrator/orchestrator.py:54 (RIGHT): The import adds MAX_PROVIDER_ERROR_BODY_BYTES, which the new test module also imports successfully, confirming the symbol is available.contextual_orchestrator/orchestrator.py:2440 (RIGHT): The changed expression reads MAX_PROVIDER_ERROR_BODY_BYTES + 1 bytes and truncates to the cap, bounding memory usage. Tests confirm the size argument is passed, oversized bodies are truncated without breaking classification, and in-bounds bodies remain parseable.tests/test_open_provider_error_body_bounds.py:105 (RIGHT): The test asserts that the transport requests exactly MAX_PROVIDER_ERROR_BODY_BYTES + 1, validating the bound is actually applied.contextual_orchestrator/orchestrator.py:2440 (RIGHT): The slice-at-cap pattern is tested for oversized bodies to confirm status and headers are preserved and classification remains correct.tests/test_open_provider_error_body_bounds.py:110 (RIGHT): The test verifies truncated oversized bodies retain status and produce correct retryability classification.tests/test_open_provider_error_body_bounds.py:119 (RIGHT): The test verifies that a normal-sized error body survives the bound and parses intact, guarding against off-by-one truncation.
Adversarial validation
contextual_orchestrator/orchestrator.py:2440 (RIGHT)falsified: A body exactly at MAX_PROVIDER_ERROR_BODY_BYTES could lose its final byte due to the slice, breaking JSON parsing. — test_in_bounds_error_body_reaches_classification_intact passes and test_error_body_read_is_bounded confirms MAX+1 bytes are requested, demonstrating the over-read preserves exactly-cap bodies.contextual_orchestrator/orchestrator.py:2440 (RIGHT)falsified: Truncating an oversized error body could cause classify_provider_failure to misclassify or crash due to invalid JSON. — test_oversized_error_body_is_truncated_but_status_preserved verifies error.code == 429 and classify_provider_failure(...).retryable is True, showing classification remains correct from the preserved status.contextual_orchestrator/orchestrator.py:2440 (RIGHT)falsified: The bound could be ineffective if the size argument is not actually passed to response.read, allowing an unbounded read. — test_error_body_read_is_bounded records the requested size and asserts it equals MAX_PROVIDER_ERROR_BODY_BYTES + 1, confirming the size argument is supplied.contextual_orchestrator/orchestrator.py:54 (RIGHT)falsified: The imported constant may not be exported from provider_errors, causing an ImportError at module load. — The new test module imports the same constant successfully, confirming the symbol is exported and importable.- Residual risk: Low. The bounded read mitigates an attacker-controlled memory amplification vector. The implementation relies on upstream HTTP handling honoring the size argument for standard http.client.HTTPResponse objects; a non-compliant transport response could still return more data, but that is outside the library's expected behavior. The +1 over-read before slicing correctly handles bodies exactly at the cap.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
d222dced2c6f578c75ffc8938bf4800cc510f3c9 - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
1 participant
Summary
response.read()inModelClient._open_provider's HTTP >= 400 branch toMAX_PROVIDER_ERROR_BODY_BYTES + 1and slices to the cap, so a hostile/misbehaving provider cannot force unbounded memory buffering on the error path.MAX_PROVIDER_ERROR_BODY_BYTES(65,536) fromprovider_errors.py; status/reason/headers are untouched, so 401/429/503 classification andretryablesemantics are unchanged.provider_error_body(exc)already uses, keeping the bound in one place.Part of the #1041 response-read bounds sweep (sibling PRs: #1127, #1128, #1129, #1131, #1132).
Tests
tests/test_open_provider_error_body_bounds.py(3 tests): read is bounded (read(65536 + 1)), oversized undeclared body is truncated while status 429 +retryable is Truesurvives, and an in-bounds body reachesprovider_error_bodyintact.assert -1 == 65537,assert 131098 <= 65536), then GREEN.python -m compileall -q contextual_orchestrator testsOK;git diff --checkclean.Summary by CodeRabbit
버그 수정
테스트