fix(privacy): enforce OpenRouter ZDR at request time - #953
Conversation
…sion ZDR eligibility is a route/model-level property, never grounds to block an entire provider account from serving. PROVIDER_MODEL_SOURCES's openrouter entry no longer sets evidence_only=True: OpenRouter was the one provider source with genuinely reliable native pricing/is_free evidence, so excluding it directly caused orchestrator/free's previously-documented structural emptiness (ADR 0041). Also fixes a backwards side effect of the old flag: _apply_discovered_model_evidence could never mark OpenRouter's own rows zdr_capable=True even when they exactly matched OpenRouter's own declared ZDR feed. The provider-neutral evidence-application contract from PR #901 (OpenRouter's feed also crediting matching rows from every other provider) is unchanged. Since OpenRouter can multiplex one model id across several backing providers, ModelClient now pins every OpenRouter request made under an active zdr_only scope with OpenRouter's own documented "provider": {"zdr": true} request-time enforcement, applied at the shared _send/_stream_send/_send_raw transport chokepoints. The async Batch API path is explicitly out of scope, stated rather than silently gapped. Also corrects documentation that had attributed this policy's original "evidence_only=True, deliberately untouched" framing, and its reversal, to fabricated or vaguely-sourced human/authority decisions ("Product direction confirmed... the owner's intent", "Per owner review on that PR") rather than stating the technical facts and the actual review that occurred.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughOpenRouter가 전역 ChangesOpenRouter ZDR 라우팅
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to The change allows OpenRouter to serve ZDR-only traffic, but configurations with no provider identity can omit the required ZDR restriction and send private inputs through an unconstrained route. Merge should wait until provider identity is validated or derived fail-closed. Sequence Diagram(s)sequenceDiagram
participant Client
participant ModelClient
participant OpenRouter
Client->>ModelClient: zdr_only 요청
ModelClient->>ModelClient: provider.zdr=true 적용
ModelClient->>OpenRouter: provider 설정과 함께 전송
OpenRouter-->>ModelClient: 응답 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 48 functions across 12 files. (6 skipped: 5 unsupported, 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 |
|
The Generated by Claude Code |
|
Investigated this PR's branch directly (not just its diff) while independently working the same ZDR-request-enforcement gap. Two findings worth flagging before merge: 1. This branch predates #949 (merged Risk: resolving that conflict without deliberately re-adding 2. Not pushing a competing branch for either of these — flagging so whoever finishes this PR can fold both in. Happy to hand over my own tested Generated by Claude Code |
…idence-only # Conflicts: # contextual_orchestrator/model_discovery.py # docs/planning/adrs/0041-generalize-models-dev-cost-classification.md # docs/product-technical-gap-baseline.md # tests/test_model_discovery.py
|
@opencode-agent please review this draft PR. Generated by Claude Code |
![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.
A late commit (fix(batch): pin OpenRouter ZDR in JSONL) added a _pin_openrouter_zdr call to _batch_run's JSONL body serialization, but the gap-baseline doc section and changelog fragment describing this PR's ZDR pinning still said the async Batch API path was explicitly out of scope and unpinned. Corrected both to describe the actual, tested behavior (test_batch_run_pins_openrouter_zdr_in_uploaded_jsonl already covers it). (Devin review on #953)
Required
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
_pin_openrouter_zdr built dict(payload.get("provider") or {})
unconditionally: a caller-supplied provider field that was present,
truthy, and not a mapping (an int, bool, list, or string) made dict()
raise a bare TypeError before any of the 5 shared call sites (chat,
streaming, tools/binary-media passthrough, batch JSONL) could handle
it as a caller error. Speech/audio requests under zdr_only surfaced
this as an unhandled failure instead of a clean validation error.
Validate provider is a dict (or None/absent) at this one shared choke
point and raise a named ValueError, matching this codebase's existing
convention for malformed caller-input fields elsewhere in
orchestrator.py (e.g. _capability_agents' "requested model ... is not
configured"). Preserves existing behavior for a valid dict provider
(merge zdr: true) and for an absent/None provider (fresh dict).
Adds regression coverage: a unit test on _pin_openrouter_zdr itself
for every non-mapping shape (int, bool, list, str, and falsy 0/""),
an integration test on the flagged proxy_send_bytes speech path, and
an HTTP-level test documenting the end-to-end response is always a
clean, well-formed JSON error envelope with no leaked Python
exception text.
Devin review on #953.
|
Generated by Claude Code |
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.
_pin_openrouter_zdr is the single choke point behind _send, _stream_send, _send_raw, proxy_send_bytes, and the non-embedding batch JSONL path, but it trusted ModelAgent.provider_name verbatim. provider_name is free-text and unvalidated at construction, so a hand-authored agent with base_url pointing at OpenRouter's own endpoint but an empty/wrong provider_name silently skipped the provider.zdr=true enforcement pin under an active zdr_only scope, even though the request still routed to OpenRouter. 944485c already closed this gap for cost_router.py's embedding-batch resolver by falling back to the base_url hostname when provider_name is empty. This applies the same normalization at the actual pinning chokepoint in orchestrator.py, so every other call site gets the same protection. Regression tests prove it end-to-end on the real _send transport, not just against the helper in isolation. CodeRabbit review on #953, discussion_r3898471887.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2f42488 fixed orchestrator.py's _resolved_openrouter_provider so the exact OpenRouter destination hostname is authoritative for the ZDR-pin decision even when provider_name is nonempty but wrong (a typo, stale copy-paste). cost_router.py's _resolved_provider_name — the equivalent choke point behind CostRoutingCoordinator._resolve_embedding_target's ZDR pin for the embedding batch path — had the identical agent.provider_name or ... short-circuit and was not touched by that fix, so an embedding agent misconfigured the same way (base_url pointing at OpenRouter, provider_name set to something else) still silently skipped provider.zdr=true under an active zdr_only scope. Applies the same base_url-hostname-first normalization used in orchestrator._resolved_openrouter_provider, scoped narrowly to _resolved_provider_name: this function's only caller is _resolve_embedding_target's ZDR-pin decision, so nothing that trusts provider_name for routing, display, credential lookup, or capability filtering elsewhere (e.g. _agent_provider_model's cost-ledger attribution) is touched. Also adds the on-wire regression coverage CodeRabbit explicitly asked for (discussion_r3898659143) that 2f42488's helper-level test didn't provide: test_send_pins_openrouter_zdr_on_the_wire_for_mistyped_provider_name proves the orchestrator.py fix on the real ModelClient._send transport (the captured outgoing JSON body), and test_openrouter_zdr_embedding_batch_overrides_mistyped_provider_name proves the new cost_router.py fix through the real CostRoutingCoordinator.submit_embeddings_batch path. Both fail against the pre-fix code and pass after. CodeRabbit review on #953, discussion_r3898471887 / discussion_r3898659143. Devin review on #953, discussion_r3898661634. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Generated by Claude Code |
|
Generated by Claude Code |
|
Cross-PR integration contract: routing identity is provider-neutral |
|
Re: the cross-PR
More importantly, there's a design-level interdependency, not just a textual conflict: #971's own gap-baseline text already claims "ZDR evidence is evaluated per discovered model rather than disabling the entire OpenRouter account" as done — but #971's actual Recommendation: this PR should merge before #971 lands, since #971 is (perhaps unknowingly) already claiming this PR's fix as done. Whoever finishes #971 will need to rebase past this PR's Generated by Claude Code |
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. |
seonghobae
changed the title
|
Stacked on #971 at exact parent The provider-wide OpenRouter Verification on the exact child tree: focused privacy/discovery/API suite |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
…er' into fix/openrouter-not-evidence-only Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Strix가 6시간 이상 동작해서 취약점 잡는 것도 본 일이 있습니다. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
|
Stack update |
|
Exact-head full regression for |
…er' into fix/openrouter-not-evidence-only Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Stack update |
|
이거 Merge 할 겁니까? 판단해요. |
60526ba
into
fix/model-group-timeout-openrouter
|
병합했습니다 (squash, Generated by Claude Code |
3 participants
Summary
PROVIDER_MODEL_SOURCES'sopenrouterentry no longer setsevidence_only=True. ZDR eligibility is a route/model-level property (is_zdr_model, exact feed matching), never grounds to block an entire provider account from serving. OpenRouter was the one provider source with genuinely reliable native pricing/is_freeevidence, so excluding it directly causedorchestrator/free's previously-documented structural emptiness (ADR 0041)._apply_discovered_model_evidencecould never mark OpenRouter's own rowszdr_capable=Trueeven when they exactly matched OpenRouter's own declared ZDR feed. The provider-neutral evidence-application contract from PR feat: route ZDR requests through discovered model groups #901 (OpenRouter's feed also crediting matching rows from every other provider) is unchanged.ModelClientnow pins every OpenRouter request made under an activezdr_onlyscope with OpenRouter's own documented"provider": {"zdr": true}request-time enforcement, applied at the shared_send/_stream_send/_send_rawtransport chokepoints. The async Batch API path is explicitly out of scope, stated rather than silently gapped.docs/product-technical-gap-baseline.md) that had attributed this policy's original framing and its reversal to vaguely-sourced authority claims ("Product direction confirmed... the owner's intent", "Per owner review on that PR") rather than stating the technical facts plainly.Test plan
python -m pytest tests -q— 2831 passed, 1 skipped (the one failure,test_fast_mlsirm_fit_uses_judge_acceptance_item_for_context_score, is a pre-existingModuleNotFoundError: No module named 'fast_mlsirm'unrelated to this change — confirmed by reproducing it againstmainbefore this branch's changes)interrogate contextual_orchestrator/model_discovery.py contextual_orchestrator/orchestrator.py— 100%test_pin_openrouter_zdr_*,test_send_pins_openrouter_zdr_on_the_wire,test_stream_send_pins_openrouter_zdr_on_the_wire,test_send_raw_pins_openrouter_zdr_on_the_wireintests/test_orchestrator_client_boundaries.py;tests/test_model_discovery.pyevidence-application assertions updated to reflect the fixed backwards-ZDR-crediting bugGenerated by Claude Code
Summary by CodeRabbit
새로운 기능
provider.zdr=true가 자동 적용됩니다.버그 수정
provider입력이 명확한 검증 오류로 거부됩니다.문서