fix(coverage-evidence): retry trusted-uv download on transient network errors - #953
fix(coverage-evidence): retry trusted-uv download on transient network errors#953seonghobae wants to merge 5 commits into
Conversation
…k errors The shared releases.astral.sh origin is fetched by every coverage-evidence run across the organization. A single transient HTTPError/URLError under concurrent load previously failed the whole job with no retry, producing false-negative REQUEST_CHANGES verdicts on otherwise-healthy PRs (observed directly on naruon #1293 and #1300, three days apart, identical error). Split _download_trusted_uv_archive into a single-attempt network sink (_fetch_trusted_uv_archive_once) plus a bounded-retry wrapper (3 attempts, short backoff) that only retries OSError. Trust-boundary violations (unsafe redirect, oversized payload) remain RuntimeErrors raised on the first attempt and are never retried. Updated the static AST security contract test to target the relocated network-sink function (same one-literal-URL invariant, unchanged). New tests cover: transient-then- success, retry exhaustion, and that redirect/size rejections are never retried. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 87 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughtrusted-uv 아카이브 다운로드를 단일 시도 함수와 재시도 제어 함수로 분리했습니다. Changestrusted-uv 다운로드 재시도
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The PR adds bounded retries for transient download failures while preserving trust checks. No actionable merge-blocking risk remains; the retry-delay assertions can be strengthened as a small follow-up. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_materialize_base_python_requirements.py (1)
552-567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win모든 재시도 지연값을 검증하십시오.
Line 564는
time.sleep인수를 버립니다. 현재 테스트는 호출 횟수만 확인합니다. 따라서 3회 실패 시 선형 지연delay,delay * 2가 상수 지연으로 변경되어도 통과합니다.수정 예시
monkeypatch.setattr(materializer.urllib.request, "urlopen", _urlopen) - monkeypatch.setattr(materializer.time, "sleep", lambda _seconds: None) + sleeps: list[float] = [] + monkeypatch.setattr(materializer.time, "sleep", sleeps.append) with pytest.raises(RuntimeError, match="download failed"): materializer._download_trusted_uv_archive() assert calls == materializer.TRUSTED_UV_DOWNLOAD_ATTEMPTS + assert sleeps == [ + materializer.TRUSTED_UV_DOWNLOAD_RETRY_DELAY_SECONDS, + materializer.TRUSTED_UV_DOWNLOAD_RETRY_DELAY_SECONDS * 2, + ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_materialize_base_python_requirements.py` around lines 552 - 567, Update test_download_trusted_uv_archive_exhausts_retries_and_reports_download_failed to record each value passed to time.sleep instead of discarding it, then assert the complete retry-delay sequence matches the expected linear delays (delay and delay * 2) for the failed attempts while preserving the existing call-count and exception assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_materialize_base_python_requirements.py`:
- Around line 552-567: Update
test_download_trusted_uv_archive_exhausts_retries_and_reports_download_failed to
record each value passed to time.sleep instead of discarding it, then assert the
complete retry-delay sequence matches the expected linear delays (delay and
delay * 2) for the failed attempts while preserving the existing call-count and
exception assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e583fc22-519c-4eab-9b3f-602232328c47
📒 Files selected for processing (4)
CHANGELOG.mdscripts/ci/materialize_base_python_requirements.pytests/test_materialize_base_python_requirements.pytests/test_trusted_uv_download_contract.py
Record each backoff sleep on retry exhaustion and require delay then 2*delay. Keep Darwin installer tests on the linux x86_64 path. Cite RFC 9110 for transient-only retries.
|
Review follow-up on current head. CodeRabbit nit is applied: exhaustion now asserts the linear backoff sequence `[delay, 2*delay]` against the shipped `time.sleep` calls. Trust-boundary violations remain unretried (1 call). Doctoring: `docs/doctoring/trusted-uv-transient-download-retry.md` (RFC 9110, APA 7th). |
CWE-755: the production urllib HTTPError is an OSError subclass and must enter the bounded retry; RuntimeError trust-boundary failures stay unretried.
A missing or forbidden archive is a client policy failure. Retry only 5xx, 429, and non-HTTP OSError so a 404 cannot be probed three times.
|
@opencode-agent @cwl-noema-review Review-only request for exact current head |
![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 could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
b983bb20091933c2f604edcaee9a6b078775edd6. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- Strix Security Scan/publish-manual-pr-evidence-status: CANCELLED (https://github.com/ContextualWisdomLab/.github/actions/runs/31686981883/job/94413986808)
- Strix Security Scan/strix: CANCELLED (https://github.com/ContextualWisdomLab/.github/actions/runs/31686981883/job/94405231262)
- Strix Security Scan/strix: cancelled (https://github.com/ContextualWisdomLab/.github/actions/runs/31686981883/job/94405231262)
- publish-manual-pr-evidence-status check run: cancelled (https://github.com/ContextualWisdomLab/.github/actions/runs/31686981883/job/94413986808)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: trusted-uv-transient-download-retry.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: trusted-uv-transient-download-retry.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: materialize_base_python_requirements.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: materialize_base_python_requirements.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test (2 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test (2 files)"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed. Findings1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
Failed checks:
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: trusted-uv-transient-download-retry.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: trusted-uv-transient-download-retry.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: materialize_base_python_requirements.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: materialize_base_python_requirements.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test (2 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test (2 files)"]
R4 --> V4["targeted test run"]
|
Materialize a base Python lock only when every package line is an exact SHA-256 pin or a two-token relative -r/--requirement include of a candidate lock path. A lone --require-hashes directive, ./dotted paths, and -r other-hashes.txt no longer enter the trusted build context.
|
@opencode-agent @cwl-noema-review Review-only request for exact current head |
|
Closing as superseded by #790. #790 implements the same buyer-visible transient trusted-uv download repair on a newer protected base, but with a narrower retry classifier (explicit transient transport conditions and HTTP 408/425/429/500/502/503/504 only), deterministic delays, immutable request reuse, trusted Git resolution, descriptor-relative no-follow output handling, symlink/hard-link/FIFO/TOCTOU defenses, full documentation, and the permanent 100% quality contract. Retrying every Do not merge both implementations; maintain one source of truth in #790. |
1 participant
Problem
coverage-evidence(used byopencode-review-dispatch.ymlto gate approval on every PR org-wide) downloads a fixeduvrelease archive fromreleases.astral.shwith zero retry. That single shared origin is fetched by every coverage-evidence run across the whole organization, so it occasionally answers a transient network/HTTP error under concurrent load even though the file itself is healthy.Observed directly, not guessed: two independent occurrences on
naruon, three days apart (PR #1293 on 2026-08-10, PR #1300 on 2026-08-13), both with the exact same log line:Both PRs were otherwise healthy — the failure produced a false-negative
REQUEST_CHANGESfromopencode-agentunrelated to either PR's actual content. This is also very likely the root cause of the twotest_materialize_base_python_requirements.py-adjacent findings already tracked as "pre-existing, environment/timing-sensitive" failures from PR #949's cycle.Fix
Split
_download_trusted_uv_archiveinto:_fetch_trusted_uv_archive_once— the single-attempt network sink, containing the entire unchanged trust boundary (redirect rejection via_RejectTrustedUvRedirects, exact host/port pin, bounded size, and the caller's checksum/member verification)._download_trusted_uv_archive— a bounded-retry wrapper (3 attempts, short linear backoff) that retries onlyOSError(the network-level failure class, which includesHTTPError/URLError). Trust-boundary violations (RuntimeErrors: unsafe redirect, oversized payload) are raised on the first occurrence and are never retried.Every retry attempt runs the identical trust boundary, so this adds resilience and does not weaken any check.
Tests
test_trusted_uv_download_contract.py) to target the relocated network-sink function — same invariant, unchanged.pytest tests/: confirmed viagit stashthat all remaining failures (13 opencode-model-pool, 5 linux-platform-guard) are byte-for-byte identical on unmodifiedmain(macOS-local-only; pass on the pinned Linux CI runner) — no new regression.interrogate: 100% docstring coverage on the changed file. Coverage: 100% reachable lines/branches outside the Linux-only platform guard, identical shape to unmodified main.Test plan
pytest tests/test_materialize_base_python_requirements.py tests/test_trusted_uv_download_contract.py -qpytest tests/ -q(full suite, confirmed no new failures vs. unmodifiedmain)interrogate -v scripts/ci/materialize_base_python_requirements.py🤖 Generated with Claude Code
Summary by CodeRabbit