fix(passthrough): virtual selectors advance past ambiguous timeouts - #1176
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 44 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: Advanced Run ID: 📒 Files selected for processing (5)
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 |
![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 reviewed the current-head product diff. Coverage is a separate gate.
Changed files
CHANGELOG.d/virtual-selector-timeout-failover.md— repository behaviorcontextual_orchestrator/orchestrator.py— Python module behaviordocs/doctoring/provider-diverse-discovery-routing.md— operator or user guidancedocs/product-technical-gap-baseline.md— operator or user guidancetests/test_passthrough_provider_failover.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: virtual-selector-timeout-failover.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: virtual-selector-timeout-failover.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python: orchestrator.py"]
S2 --> I2["Python module behavior"]
I2 --> R2["Review risk: Python: orchestrator.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Docs: provider-diverse-discovery-routing.md (2 files)"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: provider-diverse-discovery-routing.md (2 files)"]
R3 --> V3["docs review"]
Evidence --> S4["Test: test_passthrough_provider_failover.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_passthrough_provider_failover.py"]
R4 --> V4["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
c36d4a5db901a3b4449d136dbeaf7d8e95cee2b3 - Workflow run: 34758401923
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: virtual-selector-timeout-failover.md"]
S1 --> I1["repository behavior"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["required checks"]
Evidence --> S2["Python: orchestrator.py"]
S2 --> I2["Python module behavior"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["pytest plus coverage"]
Evidence --> S3["Docs: provider-diverse-discovery-routing.md (2 files)"]
S3 --> I3["operator or user guidance"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["docs review"]
Evidence --> S4["Test: test_passthrough_provider_failover.py"]
S4 --> I4["regression suite"]
I4 --> Conflict["Merge conflict blocks this path"]
Conflict --> V4["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
|
Reconciled this branch with origin/main's PR #1053. #1053's rationale (replaying an accepted request can double-bill a priced provider) applies to every selector except The candidate loop now compares Tests: kept #1053's parametrized Merge commit: a9cb8c6. 🤖 Addressed by Claude Code |
|
CI triage for head 🤖 Addressed by Claude Code |
… onto main Virtual selectors advance past a recorded ambiguous transport failure while explicit models keep fail-closed provider_outcome_unknown; rebase #1176 onto current rate-limit admission and taxonomy. Co-authored-by: Cursor <cursoragent@cursor.com>
a9cb8c6 to
a423456
Compare
… onto main Virtual selectors advance past a recorded ambiguous transport failure while explicit models keep fail-closed provider_outcome_unknown; rebase #1176 onto current rate-limit admission and taxonomy. Co-authored-by: Cursor <cursoragent@cursor.com>
a423456 to
e687905
Compare
1 participant
Defect
Noema review (org CI) calls
POST /v1/chat/completionswith the virtual modelorchestrator/free. This takes the passthrough path inTaskOrchestrator.proxy_completion(contextual_orchestrator/orchestrator.py).When the first candidate raises a
TimeoutError(90s read timeout), theambiguous-transport-failure branch recorded the failure and immediately raised
a classified
502 provider_connection_errorwithout trying the remainingready candidates — even though the free-pool preflight had three more ready
candidates.
Sidecar log evidence (run 34754423834 attempt 2, PR #1166): one
provider_attempt_failed ... TimeoutErroronnvidia_nim_deepseek_ai_deepseek_v4_pro_0813→circuit_failure→request_failed status=502 code=provider_connection_error; ready candidatesnvidia_nim_sub_deepseek_ai_deepseek_v4_pro_0813,nvidia_nim_meta_llama_3_2_11b_vision_instruct,nvidia_nim_sub_meta_llama_3_2_11b_vision_instructwere never called.This behavior was introduced for #1045 (
_is_ambiguous_passthrough_transport_failure)to stop replaying a request whose outcome is unknown (a timeout may follow
provider acceptance). That rationale is valid only for an explicit
concrete model request — the caller pinned one specific provider and nothing
else is safe to substitute. It does not hold for a virtual selector
(no model /
orchestrator/auto/orchestrator/free): the caller delegatedcandidate selection to the gateway, and
_orchestrated_provider_completion'sown docstring already documents that virtual selectors advance across
retryable transport failures (502/429/timeout). The passthrough candidate
loop contradicted that.
Change
In
TaskOrchestrator.proxy_completion's passthrough candidate loop(
contextual_orchestrator/orchestrator.py):virtual_modelflag from the samerequested_model in {None, GATEWAY_DEFAULT_MODEL, AUTO_MODEL, FREE_MODEL}check the method already uses earlier to decide whether a request even
reaches this multi-candidate loop (an explicit concrete model resolves to
one agent and returns through an earlier single-shot branch — it never
reaches this loop at all).
(
_record_failure+ groupobserve_failure— the breaker learns iteither way). When
virtual_modelis true,continueto the next rankedcandidate instead of raising, preserving
last_failureso that if everycandidate fails, the final error is still the same classified
502 provider_connection_errorshape as before.loop, so their fail-closed, single-call behavior is unchanged.
_is_ambiguous_passthrough_transport_failureandthe branch comment to state the explicit-vs-virtual distinction and why.
docs/doctoring/provider-diverse-discovery-routing.md, whichdocumented the old blanket "ambiguous timeout always fails closed" rule for
this exact passthrough pool.
Honesty / usage note: the passthrough path records no per-attempt usage rows
today (
usage_source/measurement_statusbookkeeping lives only in theorchestrated/conduct workflow path). No new usage-tracking subsystem was
added; the skipped-over candidate's unknown outcome is captured only via the
existing breaker/group-router failure observation, never as a fabricated
zero-cost usage row.
No new numeric caps were added — the loop still iterates the existing ranked,
provider-diverse candidate list with no additional bound on candidates or
time.
Tests
tests/test_passthrough_provider_failover.py:test_ambiguous_timeout_on_explicit_model_is_not_replayed— explicitconcrete model still fails closed after exactly one call (splits the old
test_ambiguous_timeout_is_not_replayed, which actually exercised theimplicit-default virtual selector, not an explicit model).
test_ambiguous_timeout_on_virtual_selector_advances_to_next_candidate(parametrized: default model,
AUTO_MODEL) — first candidate times out,second succeeds; response comes from the second; both calls recorded;
first candidate is a breaker observation.
test_ambiguous_timeout_on_free_model_advances_to_next_free_candidate—same, using
FREE_MODELwith admittedcost:freecandidates (AGENTS.mdrequires
FREE_MODELcoverage, not onlyAUTO_MODEL).test_ambiguous_timeout_on_virtual_selector_exhausts_to_classified_502—every candidate times out → classified
502 provider_connection_error,every candidate called once, all recorded in the breaker.
(
test_ambiguous_transport_failure_is_classified_and_recorded,test_ambiguous_transport_failure_is_observed_by_the_group_router) thatalso unknowingly exercised a virtual selector (
"contextual-orchestrator"/ default) and asserted the old fail-closed behavior; they now assert the
failover.
Verification
python -m pytest tests/test_passthrough_provider_failover.py tests/test_provider_reliability.py tests/test_provider_error_taxonomy.py tests/test_api_contract.py -q→ 135 passed.python -m interrogate -v contextual_orchestrator/orchestrator.py→ 100% (119/119).python -m pytest tests -q(full suite) → see PR thread / report for the summary line; one pre-existing, unrelated failure(
tests/test_privacy_policy_analysis.py::test_pinned_mcp_client_renders_and_closes_camoufox_tab,an
mcp.Clientattribute mismatch against this environment's installedmcppackage) reproduces identically on a clean
origin/maincheckout and isunrelated to this change.
Docs / changelog
CHANGELOG.d/virtual-selector-timeout-failover.mddocs/product-technical-gap-baseline.md— dated 2026-09-13 entry citing run 34754423834 / PR fix(privacy): name the MCP SDK 2.x contract for the Camoufox renderer #1166 and fix(gateway): fail over long orchestrator/free transport 502 with typed attempt evidence #1045.docs/doctoring/provider-diverse-discovery-routing.md— updated the fail-closed rule description.🤖 Generated with Claude Code