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. |
|
Fresh BandScope downstream canary for this exact ownership boundary:
The consumer is a Python dependency-security PR: its head changes the analysis-engine lock/manifest to the patched baseline ( This therefore provides a second concrete consumer for #1398's stale-base-lock class, independent of the AIP #38 reproducer. Acceptance after protected integration: rerun unchanged BandScope #1033 head |
…hon-head-lock-trust # Conflicts: # CHANGELOG.md
and removing a now-forbidden Python verdict mirror main landed substantial independent work on the scheduler/review surface while this large PR was open (183 commits behind). Reconciled: - scripts/ci/pr_review_merge_scheduler.py: kept both independently added review-dispatch-priority helpers (this branch's has_any_opencode_verdict/review_dispatch_priority/ prioritize_review_dispatch_queue) and main's can_retry_check_gated_opencode_review; combined the two competing queue-sort strategies (this branch's never-reviewed-first vs. main's stacked-PRs-first) into one sort key (baseRefName-matches-default, review_dispatch_priority) so both properties hold together. - .github/workflows/pr-review-merge-scheduler.yml: adopted main's target_default_branch fix over this branch's live_default_branch: main's design is the correct one — using the PR's own base ref (e.g. "develop" for a stacked naruon PR) as the scheduler's base_branch output would make a stacked PR compare equal to its own base and silently bypass the stacked-review dispatch budget path. Removed the now-dead repository_json/live_default_branch fetch. - .github/workflows/opencode-review-dispatch.yml / tests/test_pr_review_autofix_nvidia_nim_contract.py: kept this branch's VCS-license-validator install and pnpm packageManager helper alongside main's Python-coverage change-scoping and lock revision-tracking helper (independent, non-overlapping additions); refreshed the reviewed-workflow blob-pin hash. - opencode-review.yml: kept main's added APPROVED/CHANGES_REQUESTED review-state filter. - Removed scripts/ci/opencode_dispatch_status.py's current_head_opencode_verdict/decide_required_verdict_check and the --mode required-verdict CLI path this branch had added: main added tests/test_opencode_required_verdict_regression.py, which executes the real embedded jq gate in opencode-review.yml directly and explicitly forbids a Python "mirror" of that logic (test_required_verdict_has_one_executable_owner) so the two implementations cannot drift apart. This branch's mirror was never wired into any workflow (only the default dispatch-status mode is invoked), so it was dead code; deleted the now-obsolete tests/test_opencode_required_verdict_gate.py that unit-tested it. - Updated stale test fixtures/assertions across tests/test_opencode_workflow_shell_syntax.py, tests/test_pr_review_autofix_nvidia_nim_contract.py, and tests/test_required_workflow_queue_contract.py to match the post-merge workflow content (main independently migrated repository visibility checks from the `.private` boolean to the `.visibility` field, and the model-pool step to the contextual-orchestrator gateway loader, while this branch's tests still asserted the old shape). - CHANGELOG.md / docs/org-required-workflow-rollout.md: union merge / newer date. - Added a missing docstring on scripts/ci/validate_vcs_dependency_license.py's RejectRedirectHandler.redirect_request (pre-existing gap on this branch's own new file, caught by the 100% interrogate gate). Full suite: 1916 passed, 1 skipped, 25 subtests; coverage 100% except the pre-existing scripts/ci/pingora_edge_policy.py:274 gap already tracked and owned by PR #1398 (also visible on unmodified main); interrogate 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
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
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 의존성 검사 대상에 포함합니다.문서