fix: extend provider error boundary to streaming and batch paths - #830
Conversation
- _stream_send: mid-stream failures surface one package-owned error; the terminal tool-stop SSE contract is preserved (CWE-209) - batch_chat: upload/poll/download failures no longer leak raw urllib text - model discovery: raw connection resets (OSError, non-URLError) map to the stable transport_error code - ADR 0011: document the streaming/batch boundary extension Supersedes the still-valid delta of #807 after #771 landed the core boundary.
|
Warning Review limit reachedNext included review available in 7 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: Pro Plus Run ID: 📒 Files selected for processing (6)
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.
A provider usage-only frame can emit choices: [] or omit choices entirely. The previous raised IndexError when the key existed but the list was empty, and the broad provider error boundary would then abort an otherwise valid stream. Normalise the choices list before indexing and add a regression test covering both empty and missing choices. Devin Review: contextual-orchestrator#830 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| choices = chunk.get("choices") or [{}] | ||
| delta = (choices[0] or {}).get("delta", {}).get("content") |
There was a problem hiding this comment.
📝 Info: Explicit null delta still aborts the stream
The new choices = chunk.get("choices") or [{}] fixes the empty-list IndexError, but a frame with "delta": null still makes None.get("content") raise, which the broad except turns into a stream abort. This matches the old one-liner's behavior, so it is not a regression.
Was this helpful? React with 👍 or 👎 to provide feedback.
| except (urllib.error.URLError, TimeoutError, ValueError, OSError) as exc: | ||
| # OSError covers ConnectionError/reset failures that are not URLError | ||
| # subclasses, so a raw provider transport failure can never escape the | ||
| # discovery boundary with provider text attached. |
There was a problem hiding this comment.
📝 Info: OSError catch subsumes URLError and TimeoutError
Adding OSError makes the existing URLError and TimeoutError tuple entries redundant (both subclass OSError). Code mapping is unaffected because _provider_discovery_error_code checks the more specific types first; JSONDecodeError is a ValueError, not OSError, so it still yields invalid_response.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Merge-gate evidence (2026-08-24): Deep diff review + fixes applied; all required checks green on current head except strix (org-wide NVIDIA NIM quota exhaustion — external provider-capacity blocker; serialization fix in ContextualWisdomLab/.github#1297). Full local suite green on this head. |
# Conflicts: # contextual_orchestrator/orchestrator.py
| def _batch_run(self, agent, requests, temperature, poll_interval, poll_timeout, destination=None): # type: ignore[override] | ||
| raise RuntimeError("provider-secret-batch-body") |
There was a problem hiding this comment.
🟡 Batch boundary test never exercises the secret it claims to scrub
The RawBatchFailureClient._batch_run stub omits the effort_profile parameter that batch_chat passes positionally (_batch_run call), so the call raises TypeError before the stub body runs. The stub's RuntimeError("provider-secret-batch-body") never executes, so the assertion that this string is absent from the wrapped error is vacuous and the regression guard proves nothing.
| def _batch_run(self, agent, requests, temperature, poll_interval, poll_timeout, destination=None): # type: ignore[override] | |
| raise RuntimeError("provider-secret-batch-body") | |
| def _batch_run(self, agent, requests, temperature, poll_interval, poll_timeout, destination=None, effort_profile=None): # type: ignore[override] |
Was this helpful? React with 👍 or 👎 to provide feedback.
| except Exception as exc: # noqa: BLE001 - provider error boundary (CWE-209) | ||
| # The gateway's own terminal tool-stop contract must survive the | ||
| # boundary: convert the provider HTTP shape into the package-owned | ||
| # stop error so callers keep the 409 semantics they rely on. | ||
| if _is_tool_execution_stopped(exc): | ||
| raise _provider_tool_execution_stopped(agent) from None | ||
| raise | ||
| if isinstance(exc, ToolFallbackStoppedError): | ||
| raise |
There was a problem hiding this comment.
📝 Info: Tool-stop check safe on non-HTTP exceptions
_stream_send now runs _is_tool_execution_stopped(exc) on any exception. For non-HTTPError values the exc.read(...) call hits the AttributeError guard at _is_tool_execution_stopped and returns False, so they correctly fall through to the package-owned wrap. GeneratorExit is BaseException and is not caught.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Merge-gate evidence (2026-08-24): Deep review + integration complete; all required checks green on current head except strix (org-wide NVIDIA NIM quota exhaustion — external provider-capacity blocker; serialization fix in ContextualWisdomLab/.github#1297). Full local suite green. |
1 participant
Summary
Closes the remaining CWE-209 leak paths that survived #771: raw provider errors could still escape through (1) the SSE streaming generator, (2) the Batch API path, and (3) model discovery when a raw
ConnectionResetError/OSError(not aURLError) occurred.Changes
_stream_send: any mid-stream failure now raises one package-owned error (provider <id> streaming request failed) with cause severed. The terminal tool-stop SSE contract is preserved: tool-stop HTTP errors still convert to the package-owned stop error so callers keep 409 semantics.batch_chat: upload/poll/download failures surfaceprovider <id> batch request failedwithout raw urllib text.discover_provider_models: catchesOSError(coversConnectionError) and maps it to stabletransport_error.Supersedes the still-valid delta of #807 (closed as superseded by #771); this PR is rebased on current main.