Skip to content

chore(protocol): Use LlmClientError for stream too#137

Merged
grahamking merged 2 commits into
mainfrom
gk-stream-err-type
Jul 24, 2026
Merged

chore(protocol): Use LlmClientError for stream too#137
grahamking merged 2 commits into
mainfrom
gk-stream-err-type

Conversation

@grahamking

@grahamking grahamking commented Jul 24, 2026

Copy link
Copy Markdown
Contributor
  • In protocol differentiate between a DecodeError and a StreamError. Previously they were both Error.
  • Introduce an LlmClientError::General(String) for the unit tests to avoid using std::io::Error::other.
  • Introduce and LlmClientError::Ffi(err) for errors at the bindings boundary. That allows wrapping
    the PyErr (or Go, etc later) directly.

Fewer boxing allocations and fewer conversions. Often we can pass the LlmClientError right through.

Follow-on to #129

Assisted-by: Codex:GPT 5.6 Sol medium
Assisted-by: Claude:Opus 5 medium
Signed-off-by: Graham King grahamk@nvidia.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved streaming error handling so transport, decoding, and upstream failures retain their specific classifications.
    • Truncated or interrupted responses now surface the underlying transport error instead of a generic response error.
    • Python-originated failures preserve their original error details.
  • Improvements

    • Stream errors are now distinguished from decoding errors for clearer diagnostics.
    • Error handling is more consistent across supported streaming formats.

- In `protocol` differentiate between a `DecodeError` and a `StreamError`. Previously they were both `Error`.
- Introduce an LlmClientError::General(String) for the unit tests to avoid using `std::io::Error::other`.
- Introduce and LlmClientError::Ffi(err) for errors at the bindings boundary. That allows wrapping
the `PyErr` (or Go, etc later) directly.

Fewer boxing allocations and fewer conversions. Often we can pass the LlmClientError right through.

Assisted-by: Codex:GPT 5.6 Sol medium
Assisted-by: Claude:Opus 5 medium
Signed-off-by: Graham King <grahamk@nvidia.com>
@grahamking

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The change replaces generic boxed streaming errors with typed LlmClientError variants, distinguishes decode and upstream stream failures, preserves transport errors through translation and client layers, and updates libsy callers and Python bindings to use the revised error constructors and variants.

Changes

Typed error flow

Layer / File(s) Summary
Protocol error contracts
crates/protocol/src/client.rs, crates/protocol/src/stream.rs
LlmClientError gains Ffi and General; streaming items and aggregation return typed errors, with separate decode and upstream stream chunk variants.
Typed stream translation
crates/switchyard-translation/src/helpers.rs, crates/switchyard-translation/src/codecs/*/stream.rs
Encoding, decoding, codec lookup, SSE parsing, and IO paths now classify failures as LlmClientError; provider codecs handle both error chunk variants.
Client transport propagation
crates/libsy-llm-client/src/client.rs
Streaming client paths preserve transport, decoding, and encoding errors, with tests covering truncated JSON and SSE responses.
Libsy error integration
crates/libsy/src/error.rs, crates/libsy/src/algorithms/*, crates/libsy/src/core/algorithm.rs, crates/libsy/examples/*, crates/switchyard-py/src/libsy_bindings.rs
Libsy callers use external, test failures use General, and Python failures use Ffi.

Estimated code review effort: 4 (Complex) | ~45 minutes

Poem

I’m a rabbit with typed errors bright,
Hopping through streams from day to night.
Decode crumbs here, transport there,
No boxed mysteries in the air.
“General!” I cheer, and “Ffi!”—
The carrots compile happily.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: moving protocol streaming paths to LlmClientError.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/libsy/src/error.rs (1)

89-98: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Preserve external_boxed or provide a deprecation plan.

LibsyError::external_boxed is a public API. Removing it is a downstream-breaking change even if all repository call sites now use external. Keep a deprecated forwarding shim or document the required major-version/deprecation migration before merging.

As per coding guidelines, do not remove or rename public API exports without an explicit deprecation plan.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/libsy/src/error.rs` around lines 89 - 98, Preserve the public
LibsyError::external_boxed API while introducing or retaining external. Add
external_boxed as a deprecated forwarding shim to external, or provide an
explicit major-version deprecation and migration plan; do not remove or rename
the existing export without one.

Source: Coding guidelines

🧹 Nitpick comments (1)
crates/switchyard-translation/src/helpers.rs (1)

81-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the changed public stream APIs.

Add /// docs describing each helper’s typed error contract and streaming behavior.

As per coding guidelines, “Use /// documentation comments for public Rust items.”

Also applies to: 123-129

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-translation/src/helpers.rs` around lines 81 - 85, Document
the public stream helpers, including encode_stream and the additionally
referenced helper, with /// comments describing their streaming behavior and
typed LlmClientError contract. Place the documentation directly above each
public function without changing their signatures or implementation.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/protocol/src/client.rs`:
- Around line 93-104: Preserve deprecated compatibility variants for the removed
public APIs: in crates/protocol/src/client.rs lines 93-104, restore and
deprecate LlmClientError::Other(BoxError), directing callers toward Ffi or
General; in crates/protocol/src/stream.rs lines 118-123, restore and deprecate
LlmResponseChunk::Error, document DecodeError and StreamError, and describe the
migration path for stream consumers.

In `@crates/switchyard-translation/src/codecs/openai_chat/stream.rs`:
- Around line 50-54: Update the object-handling logic in the OpenAI stream event
decoder so objects containing an "error" field are classified as DecodeError
rather than proceeding to MessageStart or producing an empty response. Extract
and preserve the error message when available, matching the error-frame shape
emitted by the encoder around the existing error serialization logic, while
leaving normal object-shaped events unchanged.

---

Outside diff comments:
In `@crates/libsy/src/error.rs`:
- Around line 89-98: Preserve the public LibsyError::external_boxed API while
introducing or retaining external. Add external_boxed as a deprecated forwarding
shim to external, or provide an explicit major-version deprecation and migration
plan; do not remove or rename the existing export without one.

---

Nitpick comments:
In `@crates/switchyard-translation/src/helpers.rs`:
- Around line 81-85: Document the public stream helpers, including encode_stream
and the additionally referenced helper, with /// comments describing their
streaming behavior and typed LlmClientError contract. Place the documentation
directly above each public function without changing their signatures or
implementation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b4d2eecb-6f64-4b55-8959-14feab2481b0

📥 Commits

Reviewing files that changed from the base of the PR and between 5b7f4ab and 7d9ce0d.

📒 Files selected for processing (17)
  • crates/libsy-llm-client/src/client.rs
  • crates/libsy/examples/ensemble.rs
  • crates/libsy/examples/research_agent.rs
  • crates/libsy/examples/research_agent_core.rs
  • crates/libsy/examples/streaming_agent.rs
  • crates/libsy/src/algorithms/fall_through.rs
  • crates/libsy/src/algorithms/llm_class.rs
  • crates/libsy/src/core/algorithm.rs
  • crates/libsy/src/error.rs
  • crates/protocol/src/client.rs
  • crates/protocol/src/stream.rs
  • crates/switchyard-py/src/libsy_bindings.rs
  • crates/switchyard-translation/src/codecs/anthropic/stream.rs
  • crates/switchyard-translation/src/codecs/openai_chat/stream.rs
  • crates/switchyard-translation/src/codecs/responses/stream.rs
  • crates/switchyard-translation/src/codecs/stream.rs
  • crates/switchyard-translation/src/helpers.rs

Comment thread crates/protocol/src/client.rs
Comment thread crates/switchyard-translation/src/codecs/openai_chat/stream.rs
Previously that would go to MessageStart. Now it's StreamError.

Thanks Code Rabbit.

Signed-off-by: Graham King <grahamk@nvidia.com>
@grahamking
grahamking marked this pull request as ready for review July 24, 2026 21:41
@grahamking
grahamking requested a review from a team as a code owner July 24, 2026 21:41

@nachiketb-nvidia nachiketb-nvidia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks clean!

@grahamking
grahamking merged commit 5349735 into main Jul 24, 2026
16 checks passed
@grahamking
grahamking deleted the gk-stream-err-type branch July 24, 2026 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants