fix(coverage): trust validated Python head locks - #1398
Conversation
|
Warning Review limit reachedNext included review available in 33 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 (13)
📝 WalkthroughWalkthrough검증된 PR HEAD의 변경 상태에 따라 Python 잠금과 Changes현재 HEAD 잠금 구체화
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to The PR updates dependency evidence to use validated PR-head locks, but same-repository VCS dependencies can still be collapsed incorrectly and omit required imports, while large pull requests can trigger excessive policy-content downloads that delay or exhaust required CI capacity. These concrete correctness and availability risks should be addressed before merge. Sequence Diagram(s)sequenceDiagram
participant CoverageWorkflow
participant Materializer
participant GitRevision
participant UVExporter
CoverageWorkflow->>Materializer: base SHA와 head SHA 전달
Materializer->>GitRevision: 잠금과 sibling metadata 비교
GitRevision-->>Materializer: 변경 경로와 검증된 blob 반환
Materializer->>UVExporter: 변경된 uv.lock 프로젝트 HEAD export 요청
UVExporter-->>Materializer: registry 및 VCS 입력 반환
Materializer-->>CoverageWorkflow: trusted Python lock 생성
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 88.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 6 files. (6 skipped: 6 unsupported.) ✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Addressed in |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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
2ef42b67637c43097401ddbd3c3ad5878b9dc4e0. - 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/strix workflow run: cancelled (https://github.com/ContextualWisdomLab/.github/actions/runs/33245066905)
- strix: FAILURE (https://github.com/ContextualWisdomLab/.github/actions/runs/33245371857)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: opencode-review-dispatch.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: opencode-review-dispatch.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Repository file: ARCHITECTURE.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: ARCHITECTURE.md"]
R2 --> V2["required checks"]
Evidence --> S3["Repository file: CHANGELOG.md"]
S3 --> I3["repository behavior"]
I3 --> R3["Review risk: Repository file: CHANGELOG.md"]
R3 --> V3["required checks"]
Evidence --> S4["Docs: opencode-exact-vcs-dependency-evidence.md (3 files)"]
S4 --> I4["operator or user guidance"]
I4 --> R4["Review risk: Docs: opencode-exact-vcs-dependency-evidence.md (3 files)"]
R4 --> V4["docs review"]
Evidence --> S5["CI script: materialize_base_python_requirements.py"]
S5 --> I5["review and security gate shell path"]
I5 --> R5["Review risk: CI script: materialize_base_python_requirements.py"]
R5 --> V5["bash -n plus Strix self-test"]
Evidence --> S6["Test: test_materialize_base_python_requirements.py (3 files)"]
S6 --> I6["regression suite"]
I6 --> R6["Review risk: Test: test_materialize_base_python_requirements.py (3 files)"]
R6 --> V6["targeted test run"]
OpenCode Review Overview
|
…ead-lock-trust # Conflicts: # CHANGELOG.md # tests/test_pr_review_autofix_nvidia_nim_contract.py
|
Updated the existing central repair PR on exact head Local verification on the merged tree: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ead-lock-trust # Conflicts: # .github/workflows/opencode-review-dispatch.yml # CHANGELOG.md # tests/test_pr_review_autofix_nvidia_nim_contract.py
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Pushed exact-head repair
Verification on this exact local commit: No merge or approval bypass was attempted; please review the new exact head and rerun the protected checks. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Pushed exact-head follow-up
RED->GREEN regressions cover the outside-directory include and pre-Git HEAD validation. Final local verification on the resulting tree: No approval or merge bypass was attempted. |
main advanced with the owner's own parallel investigation into the same sidecar-preflight incident this branch was fixing, with far more precise evidence than this branch's own analysis had (actual hosted-run preflight/discovery artifacts, not just log-pattern reading). The real root cause turned out to be contextual_orchestrator_review_policy.py's family_cap selecting the same alphabetically-first candidates every run -- 2 of which are permanently-retired NVIDIA model ids returning HTTP 404 forever, not a transient failure -- plus a too-tight gateway smoke-test timeout and a max_tokens/probe-budget desync. All three are already fixed on main (family_cap 4->8, gateway timeout 30s->120s, max_tokens 16->4096). Conflict resolution: - CHANGELOG.md / docs/product-technical-gap-baseline.md: kept both sides' entries; revised this branch's own "correction" entry to defer to main's much more thorough "sidecar-preflight outage: consolidated evidence" entry as the authoritative root cause and fix, rather than presenting this branch's own generic "two call sites, no retry" theory as confirmed. That mechanism is still real (kept, reframed as independent/complementary), just not what caused this specific incident. - scripts/ci/contextual_orchestrator_review_sidecar.sh: kept main's new preflight-report console dump and this branch's SIDECAR_STDERR_TAIL_LINES threading -- complementary, not overlapping. Deliberate follow-up simplification in the same commit: dropped this branch's own _log_preflight_rejections (launcher.py), its sanitizer allowlist entry, and its tests -- main's own `log "sidecar preflight route evidence: ..."` fix already surfaces the same already-bounded-safe preflight_report JSON to the console, achieving the same operator-visibility goal more directly. Keeping both would have been duplicate code solving the same problem twice. Full suite: 1897 passed, 1 skipped, 21 subtests; coverage 100% on all touched files (pre-existing pingora_edge_policy.py:274 gap, owned by #1398, unaffected); interrogate 100%; bash -n clean.
…ead-lock-trust # Conflicts: # CHANGELOG.md # scripts/ci/pingora_edge_policy.py # tests/test_pingora_edge_policy.py # tests/test_pr_review_autofix_nvidia_nim_contract.py
|
Resolved the stale merge conflict against current Conflicts, in ascending complexity:
Test evidence (post-merge, in an isolated worktree):
Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…dence budget Devin Review confirmed: evaluate_pull_request applied MAX_CONTENT_REQUESTS / MAX_TOTAL_CONTENT_BYTES only to the ordinary _load_file_content scanning path. _pdf_evidence_confirms_binary made its own separate Contents API request and could read up to MAX_FILE_BYTES (1 MiB) without consulting or updating either aggregate counter, so a pull request with many patchless documentation PDFs could exhaust the required check's GitHub API quota and wall-clock budget despite the budget mechanism existing specifically to prevent that. _pdf_evidence_confirms_binary now takes the same max_bytes remaining-budget parameter _load_file_content already uses, and returns the number of raw bytes actually decoded (zero for the oversized-PDF exemption, since no content is ever fetched in that case). A new _reserve_content_budget helper centralizes the request/byte budget check so both the PDF-verification path and the ordinary scan path share identical accounting and both fail closed consistently once either budget is spent. The oversized-PDF exemption itself is preserved -- only its resource cost now counts against the shared budget. Added regression tests: many small patchless PDFs collectively exceeding MAX_TOTAL_CONTENT_BYTES, a set of PDFs crossing MAX_CONTENT_REQUESTS, and the oversized-PDF exemption itself consuming one request from the budget. 100% statement/branch coverage and 100% docstring coverage retained. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
Devin Review finding — verified and fixedRe-checked the review-comment list against the exact current head before this push. Every earlier finding on this PR was already resolved by prior commits; the one remaining open thread was the PDF budget-bypass finding, confirmed real: PDF verification bypasses evidence budgets — confirmed real, fixed
Fix: Regression tests added:
Validation_Generated by Claude Code Generated by Claude Code |
| if changed_uv_paths: | ||
| locks, vcs_manifest = _base_python_inputs( | ||
| resolved_repo, | ||
| base_sha, | ||
| excluded_uv_paths=changed_uv_paths, | ||
| ) | ||
| else: | ||
| locks, vcs_manifest = _base_python_inputs(resolved_repo, base_sha) |
There was a problem hiding this comment.
📝 Info: Changed uv projects skip stale exports
changed_uv_paths excludes changed and deleted projects before base export. A broken stale project cannot block its exact-head replacement.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Generated by Claude Code |
# Conflicts: # CHANGELOG.md # scripts/ci/pingora_edge_policy.py # tests/test_materialize_base_python_requirements.py # tests/test_pr_review_autofix_nvidia_nim_contract.py
Stale base resolved — one real "combine both" logic conflict in
|
Two conflicts: - CHANGELOG.md — kept both sides, this branch's entry above main's, matching the file's newest-first convention. - REVIEW_DISPATCH_BLOB_SHA — the pin is `git hash-object` over `.github/workflows/opencode-review-dispatch.yml`; both sides' values are correct only for their own tree (branch 9aae7f9…, main ade10b3…) and both wrong for the merge. Recomputed against the merged file: c341291. Verified after resolution: 2925 passed, 1 skipped, coverage 100%, interrogate 100%; zero conflict markers; `ruff check --select F821` clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#1932 changed `.github/workflows/opencode-review-dispatch.yml`, which REVIEW_DISPATCH_BLOB_SHA pins by whole-file `git hash-object`. The workflow auto-merged cleanly; only the pin line conflicted, and neither side's value is right for the merge: this branch c341291… (its own tree) main 26e8555… (after #1932) merged cbd0c0c… (recomputed) Verified: 2926 passed, 1 skipped, coverage 100%, interrogate 100%; zero conflict markers; `ruff check --select F821` clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
seonghobae
added
bug
ChatGPT Codex Connector
|
Fresh production RED canary from Central OpenCode dispatch run
This is not a #995 source-test failure. #995 intentionally and legitimately changes This branch already owns the intended security boundary: authenticated exact-HEAD lock selection/re-export while preserving trusted derivation, frozen/offline export, hashed/binary-only install, and a later networkless credential-free PR sandbox. Do not weaken this to “trust any PR uv.lock”. Acceptance for the current canary is instead:
If a later run reaches a separate missing-extra/test-collection failure after this fingerprint boundary is repaired, treat that as a new finding; it is not evidence for bundling speculative dependency changes into this repair now. |
2026-09-13 current-main materializer ownership correctionFresh live refs show an intervening current-main writer on the same semantic surface: #2094 is now exact That means #1398 must not be reconciled by replaying its historical materializer blobs over protected main while #2094 is active. Preserve #1398's unique exact-HEAD lock/materialization contracts and the CO #995 No source, branch ref, gate, or review state is changed by this handoff. The CO #995 canary remains acceptance evidence for the eventual reconciled #1398 owner; predecessor checks/reviews do not transfer. |
|
Fresh owner-path serialization note after re-reading the #995 canary and current materializer writers:
This does not retire or close #1398 and does not transfer #2094 checks. It only serializes two valid overlapping owner deltas so neither is lost or raced. |
|
#2094 handoff update after review repair: its live exact head is now #1398 should continue waiting for #2094's current-head settlement/normal protected integration, then ordinary/non-force reconcile its broader exact-HEAD lock contract and CO #995 changed-lock canary on top of that protected descendant. |
|
Intervening ordinary descendant adopted/strengthened the #2094 repair; treat this as the live authority, not a race. #2094 is now exact |
|
Acknowledged — holding, and not editing this branch. Confirming the constraint as I read it, so the serialization is not accidentally broken by a parallel session: no parallel edit or restack of #1398's I have no work in flight on this branch and will not start any until #2094 settles. My active surfaces are elsewhere and do not touch the materializer: #2137 (vendored orchestrator pin, held on #2139), #2139 (review runner occupancy), #2138 (CHANGELOG structure), #1884 (docs), and Generated by Claude Code |
|
New exact-head consumer evidence for this workflow/materialization owner lane:
Both fail building the trusted coverage image, before PR execution: Please include this source-layout boundary in the owner acceptance/replay for #1398 and the existing CO #995 integration chain, or delegate a bounded repair. Preserve locked provenance, isolation and compiled-artifact controls; do not skip tests or infer that a source-only path substitutes for building declared native dependencies. First establish the locked source layout and its build contract, then validate image construction and replay these unchanged heads after owner integration. No central source or workflow was changed from this consumer work. Next review: 2026-09-14 because this blocks existing security repairs. |
2 participants
Summary
uv.lockprojects from exact HEAD, including registry and VCS inputs;PR_HEAD_SHAthrough the central OpenCode coverage workflow; andRoot cause
coverage-evidencebuilt its trusted Python image only fromPR_BASE_SHA. A dependency PR such asContextualWisdomLab/accounting-information-platform#38could add the Python 3.14 wheel hash while the builder still consumed the older base lock, so pip failed before target tests with a stale hash set. Unchanged parent requirements locks could likewise retain stale included locks. Changed or deleteduv.lockprojects could retain stale base registry and VCS exports, and collapsing shared VCS ownership could hide an unaffected source or conflict.Security boundary
Only regular candidate lock blobs read from the validated HEAD can replace a base candidate. They must be flat, exact
==requirements with complete SHA-256 hashes. Bounded includes beneath unchanged parents are compared by exact base/head blob identity and read from HEAD only when they remain regular, flat, and fully pinned. Changeduv.lockprojects use the existing isolated frozen/offline exporter; URLs, unpinned content, source distributions, and lifecycle execution remain outside this path. The image install retainspip install --require-hashes --only-binary=:all:, and the later PR sandbox remains networkless and credential-free.Verification
183 passed, plus both exhausted and crossing-file aggregate byte-budget cases;1905 passed, 1 skipped, 17 subtests passed;100%(10072/10072statements,3982/3982branches);100%;git diff --checkpassed;8c0056529e68dbe9184cd4019a1f3d8f3a4ead2f6fc7a5afcf27d3314edd1277.Live non-passing evidence (2026-08-30 KST)
ca084e7c12066f54b86754c1548baec5ea95a5b3on protectedmain@6c8ee24046d743b3981c566c6e29f99f09137f6a.ca084e7c; Trivy and Semgrep report 0 blocking findings, and OSV reports 0 results.CHANGES_REQUESTEDreviews do not satisfy this head.Refs #1250Summary by CodeRabbit
개선 사항
.txt변경을 Python 의존성 검사 대상에 포함합니다.문서