fix(ci): apply unambiguous OpenRouter ZDR evidence - #1382
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughOpenRouter 피드는 정확한 라우트 멤버십과 다른 공급자 모델의 명확한 식별자를 ZDR 증거로 사용합니다. OpenRouter 무료 경로는 라우팅 카탈로그에서 제외합니다. 모든 리뷰 경로에 ZDR 요구를 적용하고 CI 입력 검증과 ChangesZDR 증거 및 필수 라우팅
CI 호환성 및 입력 검증
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to This change broadens ZDR classification using public endpoint evidence, but malformed feed identifiers can cause false privacy classifications and non-OpenRouter routes may be marked ZDR without provider-specific proof; the documentation also contains an ambiguous date reference. These issues should be resolved before merging. Sequence Diagram(s)sequenceDiagram
participant ReviewWorkflow
participant Sidecar
participant OpenRouterFeed
participant Gateway
ReviewWorkflow->>Sidecar: CONTEXTUAL_ORCHESTRATOR_REQUIRE_ZDR=true 전달
Sidecar->>OpenRouterFeed: Bearer 인증으로 ZDR 피드 조회
Sidecar->>Gateway: ZDR 필수 정책과 카탈로그 전달
Gateway-->>ReviewWorkflow: ZDR 경로 기반 리뷰 실행
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 90.91% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 55 functions across 19 files. (7 skipped: 6 unsupported, 1 too large.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
Current-head review request for Scope: Adversarial probes:
Please publish a structured current-head approval or changes-requested review naming this SHA and |
![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.
![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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current HEAD OpenRouter documents
Validation at this HEAD: targeted Ruff passed, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
[@opencode-agent] Review current PR HEAD |
seonghobae
changed the title
|
[@opencode-agent] Re-run the current-head review now that the summary has been corrected and the only unresolved thread is resolved. Review |
# Conflicts: # scripts/ci/zdr_policy.py
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
seonghobae
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current HEAD |
|
[@opencode-agent] Review only current HEAD |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This reverts commit 2e77b23.
|
At current HEAD |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Attempted the merge of this PR against current What resolves cleanly (verified, not yet pushed)Nine files conflicted:
Broader validation: What's blocking: is OpenRouter's own discovered route ever a legitimate served candidate?This PR adds
With this PR's exclusion applied, those two
Whichever way this should go, could you (or whoever owns this PR) confirm the intended answer? I'm happy to push the rest of this merge (already fully worked out above) the moment that's settled — it's the only open question. Generated by Claude Code |
Merge conflict resolved
Conflicting files and resolution
No hunk required rule-4 escalation — every conflict was resolvable with clear evidence for which side was authoritative or how to combine both. Verification (from
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…to origin repo materialize_base_python_requirements.py's materialize() downloaded every uv-export-pinned organization archive unconditionally, even one whose own PEP 508 marker already excludes it for the coverage image's pinned Python 3.14 (e.g. python_version == '3.11'). A transient outage on that irrelevant archive's URL failed the whole coverage-evidence job for no reason, since the image would never install it anyway. Add a minimal, stdlib-only (ast-based) evaluator for python_version/python_full_version markers and use it, together with a new --target-python-version flag wired from the workflow's pinned coverage image tag, to skip the download only when the marker can be confidently proven false; anything it cannot evaluate still downloads exactly as before, so this never drops real dependency coverage. Separately, harden _TrustedOrgArchiveRedirects: it verified that a redirect stayed on the github.com/codeload.github.com allowlist but not that it named the same repository, so a same-host redirect from github.com/ContextualWisdomLab/<repo> to codeload.github.com/<other-org>/<other-repo> would have been followed (SHA-256 verification of the downloaded bytes against the pinned hash makes this impractical to exploit today, but the check should not rely on that alone). Add an owner/repo-aware comparison used both in redirect_request and as a defense-in-depth check on the final resolved URL.
…argo fetch loop Two bugs in the coverage tool image's Dockerfile heredoc (embedded in opencode-review-dispatch.yml's "Measure test and docstring evidence" step): - The archive-manifest.json read at image-build time was unconditional (json.loads(path.read_text(...))), but the "no Python source or dependency-manifest changes" skip path never creates that file (only manifest.json/manifest.txt/vcs-manifest.json). Any PR touching no Python files therefore crashed the coverage image build. Make the read tolerant of a missing manifest, treating it as an empty archive list, matching the existing convention in install_base_python_locks.py's _archive_entries(). - The Cargo dependency cache warm-up used `while IFS= read -r -d "" ...` to walk `find -print0` output, but python:3.14-slim's default shell for Docker's shell-form RUN is dash, and dash's `read` does not implement bash's `-d` extension. Under dash this `read` fails immediately with "Illegal option -d", the loop body (cargo fetch) never runs, and because a failing while-condition is not itself a `set -e` trigger, the RUN step still reports success -- leaving the Cargo archive cache empty and only surfacing later as a confusing offline-build failure. Replace the loop with `xargs -0 -r -n1`, which is POSIX-sh portable and (unlike the original) also propagates a real cargo fetch failure through set -e. Both were verified against a real dash before and after the fix.
|
Triaged and fixed all 4 Devin findings on this PR. Pushed as two commits on top of
All 4 were real (no false positive this round). Summary per finding: Finding 1 — unconditional archive download, fixedConfirmed: Fix: added a minimal, stdlib-only ( Regression evidence: Finding 2 — redirect host-only allowlist, fixed (low severity, hardened anyway)Confirmed the mechanism: Fix: added Regression evidence: Finding 3 — missing archive-manifest.json crashes the coverage image build, fixedConfirmed: the "no Python source or dependency-manifest changes" skip path (workflow line ~671 pre-fix) creates Regression evidence: extended Finding 4 — dash-incompatible
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…m a major.minor target _evaluate_marker_node fed the coverage workflow's major.minor --target-python-version (e.g. "3.14") into the same zero-padded comparison path for both python_version and python_full_version marker variables. Zero-padding "3.14" into "3.14.0" is correct for python_version (which genuinely is major.minor) but silently assumes a patch release of .0 for python_full_version, which is patch- sensitive and not something the target string actually claims. That let patch-sensitive comparisons resolve confidently and wrongly, e.g. `python_full_version >= "3.14.1"` evaluated to False against the padded "3.14.0" even though the real interpreter (say 3.14.5) would make the marker True -- causing a needed archive to be silently skipped before the Docker build. Add a python_full_version-specific guard: only resolve such a comparison confidently when the target string itself already carries patch precision (3+ dotted components); otherwise raise _UnsupportedMarkerError so the caller keeps its existing fail-open default (download, never skip). python_version comparisons are unaffected since that variable's target value is already exact. Add regression coverage for both an equality and an inequality python_full_version marker against a major.minor-only target (must not confidently skip), a confirmation that python_full_version resolves confidently once the target is patch-precise, and keep the existing python_version confidence tests.
Devin's follow-up bug: confirmed and fixedVerification. Read Root cause: the Fix ( Regression tests (
Reproduced first: ran the new/updated cases against the pre-fix code and confirmed the buggy Full verification (clean): Note: Base at push time: Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… series _evaluate_marker_node previously failed open for every python_full_version comparison whenever the target was major.minor-only, even when the marker's literal falls in a different major.minor series entirely (e.g. python_full_version == "3.9.0" or < "3.0.0" against a "3.14" target). Those are decidable for every possible patch of the target's minor series, since the major.minor mismatch alone settles the comparison. Add _python_full_version_comparison_is_ambiguous to scope the fail-open path to the genuinely unknown case: the literal's major.minor matches the target's. Only then does the outcome depend on the interpreter's real, not-yet-known patch digit, for any operator. Update the marker-evaluation regression tests: the previously-failing-open cross-minor case now resolves confidently, with new cases covering an out-of-series lower/upper bound, a literal-on-the-left comparison, and a short literal that zero-pads into a different major.minor. The existing same-minor-series cases (which must keep failing open) are unchanged.
Refinement: confidently-decidable
|
| target_tuple = _marker_version_tuple(target_python_version) | ||
| literal_tuple = _marker_version_tuple(literal) | ||
| known_length = len(target_tuple) | ||
| literal_prefix = literal_tuple[:known_length] | ||
| literal_prefix = literal_prefix + (0,) * (known_length - len(literal_prefix)) | ||
| return literal_prefix == target_tuple |
There was a problem hiding this comment.
📝 Info: Cross-minor comparisons remain patch-independent
_python_full_version_comparison_is_ambiguous rejects only matching major.minor prefixes. Different prefixes settle every operator without the unknown patch; matching prefixes retain the archive.
Was this helpful? React with 👍 or 👎 to provide feedback.
Attempted merge, aborted — genuine architectural divergence in the ZDR/pricing admission model, same class as
|
Eight conflict blocks across five files. The sidecar change — this PR's
authenticated ZDR feed fetch — is kept. Three files were taken from main
wholesale because this branch's version contradicts a documented, newer main
decision; each is recorded below so it can be re-argued rather than lost.
KEPT — scripts/ci/contextual_orchestrator_review_sidecar.sh
The feed fetch now carries `-H "Authorization: Bearer ${OPENROUTER_API_KEY}"`
behind an `[ -n … ]` guard, so it degrades to the dated static attestation
table when the key is absent. `OPENROUTER_API_KEY` is wired into
noema-review.yml:643, opencode-review-dispatch.yml:2530 and
pr-review-autofix.yml:271, so the authenticated path is reachable.
`--max-time 15` was dropped from that same line. Two contract tests
(test_contextual_orchestrator_review_runtime_preflight.py,
test_contextual_orchestrator_sidecar_unbounded_wait_contract.py) assert these
curl calls carry no wall-clock timeout; the branch block mixed the wanted
auth header with a forbidden deadline.
TAKEN FROM MAIN — scripts/ci/contextual_orchestrator_review_policy.py
This branch adds `EVIDENCE_ONLY_PROVIDERS = frozenset({"openrouter"})`,
excluding OpenRouter from routing and reporting `evidence_only_free_routes`.
Main's `FREE_POOL_CREDENTIAL_NAMES` explicitly lists `OPENROUTER_API_KEY` as
authorized to contribute to `orchestrator/free`, and its docstring records
that only `OPENAI_API_KEY` is intentionally absent. The two designs are
mutually exclusive and each side's tests pin its own: keeping the branch's
broke 7 of main's, and taking main's code alone broke 7 of the branch's.
Main's is newer and documented, so both the module and
tests/test_contextual_orchestrator_review_policy.py were taken from main.
This also drops `PROVIDER_FAMILIES` (provider-family diversity), which main
replaced with credential-account diversity — main references
`free_account_diversity` in four test files and `provider_family` in none.
TAKEN FROM MAIN — docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md
This branch deletes the Decision line "Zero Data Retention (ZDR)-compliant
routes remain mandatory for private targets", which
test_strix_required_smoke_availability.py pins as a required-workflow
contract. Main's ADR delta since the merge base is +107/-27 against this
branch's +44/-37. The branch's replacement text also described a
canonical-`openrouter/<model>`-feed-key rule that has no implementation on
either side (`canonical` appears zero times in the policy module on both),
so it was not carried across.
CHANGELOG.md kept both sides. REVIEW_DISPATCH_BLOB_SHA recomputed against the
merged `opencode-review-dispatch.yml`: a6fa24a.
The branch's remaining delta against main is 33 files, +2345/-190 — the
narrowing above removes the contested policy design, not the PR.
Verified against this branch's unmerged head, comparing failure names:
unmerged head 0 failed, 2031 passed
after merge 0 failed, 2991 passed introduced: 0
coverage 100%, interrogate 100%, `ruff --select F821` clean, `bash -n` clean,
zero conflict markers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Resolved the conflicts with Kept — the authenticated ZDR feed fetch. That is this PR's title change and it survives intact, minus Dropped — I took main's module and its The same choice dropped Dropped — this branch's rewrite of ADR-0003. It deletes the Decision line "Zero Data Retention (ZDR)-compliant routes remain mandatory for private targets", which This does not gut the PR. The remaining delta against main is 33 files, 🤖 Generated with Claude Code |
… mutually exclusive designs Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae
added
bug
ChatGPT Codex Connector
2 participants
Summary
zdr_onlyas a boolean selection policy over the caller-supplied model-group candidate arrayNo confidential
naruon/tests/real_datasetscontent was accessed or uploaded.Verification
pytest -q tests/test_zdr_policy.py tests/test_contextual_orchestrator_review_policy.py— 59 passedgit diff --check— passedOpenRouter documents
/api/v1/endpoints/zdras an endpoint-level feed: https://openrouter.ai/docs/guides/features/zdrSummary by CodeRabbit
개선 사항
버그 수정
문서