chore(libsy): lib and proto clean up - prepare for llm-client PR#76
Merged
Conversation
WalkthroughThe change replaces the legacy LLM client interface with routed client traits, propagates request context through offloads, normalizes example responses to aggregated text, and updates orchestration tests and documentation. ChangesRouted LLM migration
Estimated code review effort: 3 (Moderate) | ~30 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/switchyard-translation/src/codecs/stream.rs (1)
200-220: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the newly exposed streaming contract.
crates/switchyard-translation/src/codecs/stream.rs#L200-L220: add concise docs describing decode/encode direction and error fallback behavior.crates/switchyard-translation/src/lib.rs#L18-L18: document that this re-export is the normalized provider-independent streaming chunk type.As per coding guidelines,
crates/**/*.rspublic items must have concise comments/doc comments.🤖 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/codecs/stream.rs` around lines 200 - 220, Document the public decode_stream_event and encode_stream_event functions in crates/switchyard-translation/src/codecs/stream.rs, concisely stating their provider-format-to-neutral and neutral-to-provider directions and that decode failures produce an error response chunk. Document the LlmResponseChunk re-export in crates/switchyard-translation/src/lib.rs as the normalized provider-independent streaming chunk type.Source: Coding guidelines
crates/libsy-protocol/src/llm.rs (1)
234-249: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new public response types.
StopReasonandResponseOutputare public API additions but currently lack rustdoc. Add concise documentation describing normalized stop reasons and response-output semantics.Proposed documentation
+/// Provider-neutral reason why generation stopped. pub enum StopReason { EndTurn, MaxTokens, ToolUse, ContentFilter, Error, Unknown, } +/// One role-tagged output from a buffered response. pub struct ResponseOutput { pub role: Role, pub content: Vec<ContentBlock>, pub stop_reason: Option<StopReason>, }As per coding guidelines,
crates/**/*.rsmust add concise comments/doc comments for public items and non-obvious helpers.🤖 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-protocol/src/llm.rs` around lines 234 - 249, Add concise rustdoc comments to the public StopReason enum and ResponseOutput struct. Describe StopReason as the normalized reason generation stopped, and ResponseOutput as one normalized assistant response item, including its role, content blocks, and optional stop reason.Source: Coding guidelines
🧹 Nitpick comments (2)
crates/libsy-protocol/src/envelope.rs (1)
10-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public
ContextAPI.Add concise docs explaining how
valuespropagates and any conventions for keys. As per coding guidelines, “Add concise comments/doc comments for public items and non-obvious helpers.”🤖 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-protocol/src/envelope.rs` around lines 10 - 13, Document the public Context struct and its values field with concise Rust doc comments, describing how values propagate and specifying the key-naming conventions callers should follow.Source: Coding guidelines
crates/libsy/src/lib.rs (1)
125-140: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the entire routed request into the promise wrapper.
inneralready retains the boxedRoutedRequest, sorouted.clone()duplicates normalized messages, metadata, and potentially largeraw_requestJSON for every outstanding call. Read it frominneron demand or store the payload behind anArc.🤖 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/lib.rs` around lines 125 - 140, The CallLlmRequest::new constructor currently clones the entire RoutedRequest into the wrapper, duplicating potentially large payload data. Remove the owned routed field and read the existing RoutedRequest from inner on demand through the accessors, or retain shared ownership with Arc if needed; preserve the current payload validation behavior without duplicating it.
🤖 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/libsy-examples/examples/streaming_agent.rs`:
- Around line 88-92: Update the chunk handling loop in streaming_agent so
LlmResponseChunk::Error is matched explicitly and propagated instead of being
ignored, while preserving text output handling. Also replace the ignored stdout
flush result with propagation of its failure from the surrounding async
function.
In `@crates/libsy-examples/src/llm_class.rs`:
- Around line 114-120: Clear stale raw_request payloads when constructing the
synthetic classifier request in llm_class.rs around lines 114-120, using None or
regenerating it with the classifier preamble. Apply the same change to the
synthetic judge request in ensemble.rs around lines 273-277, ensuring both
routed requests use their newly generated prompts instead of inherited provider
payloads.
- Around line 137-144: The response-processing algorithms incorrectly use agg(),
which ignores streaming responses. In crates/libsy-examples/src/llm_class.rs
lines 137-144, replace this with aggregate().await? before parsing the
classifier score; in crates/libsy-examples/src/ensemble.rs lines 278-289,
aggregate the judge response before parsing its selection; and in
crates/libsy-examples/src/ensemble.rs lines 334-342, aggregate candidate
responses before constructing the judge prompt and retain each aggregate as the
potential winner response.
In `@crates/libsy-protocol/src/stream.rs`:
- Around line 151-173: Update the response accumulator around the
LlmResponseChunk handling and its emission logic to retain each text, reasoning,
and tool-call content block under its streamed index, rather than merging by
type. Emit the accumulated blocks in ascending index order so interleaved
sequences such as text → tool call → text preserve their original order and can
round-trip through provider encoders.
- Around line 175-177: Update the LlmResponseChunk::MessageStop handling to only
assign stop_reason when the event provides a reason; preserve any previously
stored MaxTokens or ToolUse value when reason is absent, while retaining the
existing EndTurn mapping for explicit reasons.
In `@crates/libsy/README.md`:
- Around line 82-100: The RoutedLlmClient example in README.md is not
self-contained because its required types, helper, and async-trait dependency
are unspecified. Add the necessary imports for Decision, Response, LlmResponse,
and text_response from their actual modules, and document that consumers must
declare async-trait as a direct dependency before using the async_trait
attribute.
- Around line 237-239: Update the score extraction around
classify_response.llm_response.aggregate().await to handle aggregation errors
explicitly instead of silently converting them with .ok(). Match the Err case
according to the intended fail-open behavior, or propagate it with ?; preserve
the existing parsing and no-score handling for successful aggregates.
In `@crates/switchyard-translation/src/codecs/anthropic/stream.rs`:
- Around line 89-109: Update the `message_delta` handling to only store
`stop_reason` in `state.stop_reason` without pushing
`LlmResponseChunk::MessageStop`. Keep the single `MessageStop` emission in the
`message_stop` branch, using the remembered reason so direct consumers receive
completion exactly once.
---
Outside diff comments:
In `@crates/libsy-protocol/src/llm.rs`:
- Around line 234-249: Add concise rustdoc comments to the public StopReason
enum and ResponseOutput struct. Describe StopReason as the normalized reason
generation stopped, and ResponseOutput as one normalized assistant response
item, including its role, content blocks, and optional stop reason.
In `@crates/switchyard-translation/src/codecs/stream.rs`:
- Around line 200-220: Document the public decode_stream_event and
encode_stream_event functions in
crates/switchyard-translation/src/codecs/stream.rs, concisely stating their
provider-format-to-neutral and neutral-to-provider directions and that decode
failures produce an error response chunk. Document the LlmResponseChunk
re-export in crates/switchyard-translation/src/lib.rs as the normalized
provider-independent streaming chunk type.
---
Nitpick comments:
In `@crates/libsy-protocol/src/envelope.rs`:
- Around line 10-13: Document the public Context struct and its values field
with concise Rust doc comments, describing how values propagate and specifying
the key-naming conventions callers should follow.
In `@crates/libsy/src/lib.rs`:
- Around line 125-140: The CallLlmRequest::new constructor currently clones the
entire RoutedRequest into the wrapper, duplicating potentially large payload
data. Remove the owned routed field and read the existing RoutedRequest from
inner on demand through the accessors, or retain shared ownership with Arc if
needed; preserve the current payload validation behavior without duplicating it.
🪄 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: 8c5155af-7655-4302-bef3-1617aa2bf626
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (28)
crates/libsy-examples/Cargo.tomlcrates/libsy-examples/examples/research_agent.rscrates/libsy-examples/examples/research_agent_core.rscrates/libsy-examples/examples/streaming_agent.rscrates/libsy-examples/src/ensemble.rscrates/libsy-examples/src/llm_class.rscrates/libsy-examples/src/rand.rscrates/libsy-protocol/Cargo.tomlcrates/libsy-protocol/src/client.rscrates/libsy-protocol/src/envelope.rscrates/libsy-protocol/src/lib.rscrates/libsy-protocol/src/llm.rscrates/libsy-protocol/src/stream.rscrates/libsy/README.mdcrates/libsy/src/driver.rscrates/libsy/src/lib.rscrates/switchyard-translation/src/codecs/anthropic/buffered.rscrates/switchyard-translation/src/codecs/anthropic/stream.rscrates/switchyard-translation/src/codecs/mod.rscrates/switchyard-translation/src/codecs/openai_chat/buffered.rscrates/switchyard-translation/src/codecs/openai_chat/stream.rscrates/switchyard-translation/src/codecs/responses/buffered.rscrates/switchyard-translation/src/codecs/responses/stream.rscrates/switchyard-translation/src/codecs/stream.rscrates/switchyard-translation/src/engine.rscrates/switchyard-translation/src/lib.rscrates/switchyard-translation/tests/extension_points.rscrates/switchyard-translation/tests/stream_translation.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/switchyard-translation/src/codecs/stream.rs (1)
200-220: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the newly exposed streaming contract.
crates/switchyard-translation/src/codecs/stream.rs#L200-L220: add concise docs describing decode/encode direction and error fallback behavior.crates/switchyard-translation/src/lib.rs#L18-L18: document that this re-export is the normalized provider-independent streaming chunk type.As per coding guidelines,
crates/**/*.rspublic items must have concise comments/doc comments.🤖 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/codecs/stream.rs` around lines 200 - 220, Document the public decode_stream_event and encode_stream_event functions in crates/switchyard-translation/src/codecs/stream.rs, concisely stating their provider-format-to-neutral and neutral-to-provider directions and that decode failures produce an error response chunk. Document the LlmResponseChunk re-export in crates/switchyard-translation/src/lib.rs as the normalized provider-independent streaming chunk type.Source: Coding guidelines
crates/libsy-protocol/src/llm.rs (1)
234-249: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new public response types.
StopReasonandResponseOutputare public API additions but currently lack rustdoc. Add concise documentation describing normalized stop reasons and response-output semantics.Proposed documentation
+/// Provider-neutral reason why generation stopped. pub enum StopReason { EndTurn, MaxTokens, ToolUse, ContentFilter, Error, Unknown, } +/// One role-tagged output from a buffered response. pub struct ResponseOutput { pub role: Role, pub content: Vec<ContentBlock>, pub stop_reason: Option<StopReason>, }As per coding guidelines,
crates/**/*.rsmust add concise comments/doc comments for public items and non-obvious helpers.🤖 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-protocol/src/llm.rs` around lines 234 - 249, Add concise rustdoc comments to the public StopReason enum and ResponseOutput struct. Describe StopReason as the normalized reason generation stopped, and ResponseOutput as one normalized assistant response item, including its role, content blocks, and optional stop reason.Source: Coding guidelines
🧹 Nitpick comments (2)
crates/libsy-protocol/src/envelope.rs (1)
10-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public
ContextAPI.Add concise docs explaining how
valuespropagates and any conventions for keys. As per coding guidelines, “Add concise comments/doc comments for public items and non-obvious helpers.”🤖 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-protocol/src/envelope.rs` around lines 10 - 13, Document the public Context struct and its values field with concise Rust doc comments, describing how values propagate and specifying the key-naming conventions callers should follow.Source: Coding guidelines
crates/libsy/src/lib.rs (1)
125-140: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the entire routed request into the promise wrapper.
inneralready retains the boxedRoutedRequest, sorouted.clone()duplicates normalized messages, metadata, and potentially largeraw_requestJSON for every outstanding call. Read it frominneron demand or store the payload behind anArc.🤖 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/lib.rs` around lines 125 - 140, The CallLlmRequest::new constructor currently clones the entire RoutedRequest into the wrapper, duplicating potentially large payload data. Remove the owned routed field and read the existing RoutedRequest from inner on demand through the accessors, or retain shared ownership with Arc if needed; preserve the current payload validation behavior without duplicating it.
🤖 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/libsy-examples/examples/streaming_agent.rs`:
- Around line 88-92: Update the chunk handling loop in streaming_agent so
LlmResponseChunk::Error is matched explicitly and propagated instead of being
ignored, while preserving text output handling. Also replace the ignored stdout
flush result with propagation of its failure from the surrounding async
function.
In `@crates/libsy-examples/src/llm_class.rs`:
- Around line 114-120: Clear stale raw_request payloads when constructing the
synthetic classifier request in llm_class.rs around lines 114-120, using None or
regenerating it with the classifier preamble. Apply the same change to the
synthetic judge request in ensemble.rs around lines 273-277, ensuring both
routed requests use their newly generated prompts instead of inherited provider
payloads.
- Around line 137-144: The response-processing algorithms incorrectly use agg(),
which ignores streaming responses. In crates/libsy-examples/src/llm_class.rs
lines 137-144, replace this with aggregate().await? before parsing the
classifier score; in crates/libsy-examples/src/ensemble.rs lines 278-289,
aggregate the judge response before parsing its selection; and in
crates/libsy-examples/src/ensemble.rs lines 334-342, aggregate candidate
responses before constructing the judge prompt and retain each aggregate as the
potential winner response.
In `@crates/libsy-protocol/src/stream.rs`:
- Around line 151-173: Update the response accumulator around the
LlmResponseChunk handling and its emission logic to retain each text, reasoning,
and tool-call content block under its streamed index, rather than merging by
type. Emit the accumulated blocks in ascending index order so interleaved
sequences such as text → tool call → text preserve their original order and can
round-trip through provider encoders.
- Around line 175-177: Update the LlmResponseChunk::MessageStop handling to only
assign stop_reason when the event provides a reason; preserve any previously
stored MaxTokens or ToolUse value when reason is absent, while retaining the
existing EndTurn mapping for explicit reasons.
In `@crates/libsy/README.md`:
- Around line 82-100: The RoutedLlmClient example in README.md is not
self-contained because its required types, helper, and async-trait dependency
are unspecified. Add the necessary imports for Decision, Response, LlmResponse,
and text_response from their actual modules, and document that consumers must
declare async-trait as a direct dependency before using the async_trait
attribute.
- Around line 237-239: Update the score extraction around
classify_response.llm_response.aggregate().await to handle aggregation errors
explicitly instead of silently converting them with .ok(). Match the Err case
according to the intended fail-open behavior, or propagate it with ?; preserve
the existing parsing and no-score handling for successful aggregates.
In `@crates/switchyard-translation/src/codecs/anthropic/stream.rs`:
- Around line 89-109: Update the `message_delta` handling to only store
`stop_reason` in `state.stop_reason` without pushing
`LlmResponseChunk::MessageStop`. Keep the single `MessageStop` emission in the
`message_stop` branch, using the remembered reason so direct consumers receive
completion exactly once.
---
Outside diff comments:
In `@crates/libsy-protocol/src/llm.rs`:
- Around line 234-249: Add concise rustdoc comments to the public StopReason
enum and ResponseOutput struct. Describe StopReason as the normalized reason
generation stopped, and ResponseOutput as one normalized assistant response
item, including its role, content blocks, and optional stop reason.
In `@crates/switchyard-translation/src/codecs/stream.rs`:
- Around line 200-220: Document the public decode_stream_event and
encode_stream_event functions in
crates/switchyard-translation/src/codecs/stream.rs, concisely stating their
provider-format-to-neutral and neutral-to-provider directions and that decode
failures produce an error response chunk. Document the LlmResponseChunk
re-export in crates/switchyard-translation/src/lib.rs as the normalized
provider-independent streaming chunk type.
---
Nitpick comments:
In `@crates/libsy-protocol/src/envelope.rs`:
- Around line 10-13: Document the public Context struct and its values field
with concise Rust doc comments, describing how values propagate and specifying
the key-naming conventions callers should follow.
In `@crates/libsy/src/lib.rs`:
- Around line 125-140: The CallLlmRequest::new constructor currently clones the
entire RoutedRequest into the wrapper, duplicating potentially large payload
data. Remove the owned routed field and read the existing RoutedRequest from
inner on demand through the accessors, or retain shared ownership with Arc if
needed; preserve the current payload validation behavior without duplicating it.
🪄 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: 8c5155af-7655-4302-bef3-1617aa2bf626
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (28)
crates/libsy-examples/Cargo.tomlcrates/libsy-examples/examples/research_agent.rscrates/libsy-examples/examples/research_agent_core.rscrates/libsy-examples/examples/streaming_agent.rscrates/libsy-examples/src/ensemble.rscrates/libsy-examples/src/llm_class.rscrates/libsy-examples/src/rand.rscrates/libsy-protocol/Cargo.tomlcrates/libsy-protocol/src/client.rscrates/libsy-protocol/src/envelope.rscrates/libsy-protocol/src/lib.rscrates/libsy-protocol/src/llm.rscrates/libsy-protocol/src/stream.rscrates/libsy/README.mdcrates/libsy/src/driver.rscrates/libsy/src/lib.rscrates/switchyard-translation/src/codecs/anthropic/buffered.rscrates/switchyard-translation/src/codecs/anthropic/stream.rscrates/switchyard-translation/src/codecs/mod.rscrates/switchyard-translation/src/codecs/openai_chat/buffered.rscrates/switchyard-translation/src/codecs/openai_chat/stream.rscrates/switchyard-translation/src/codecs/responses/buffered.rscrates/switchyard-translation/src/codecs/responses/stream.rscrates/switchyard-translation/src/codecs/stream.rscrates/switchyard-translation/src/engine.rscrates/switchyard-translation/src/lib.rscrates/switchyard-translation/tests/extension_points.rscrates/switchyard-translation/tests/stream_translation.rs
🛑 Comments failed to post (8)
crates/libsy-examples/examples/streaming_agent.rs (1)
88-92: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Propagate in-band stream errors instead of silently ignoring them.
LlmResponseChunk::Errorcurrently falls through theif let, so the example reports successful output after a provider error. Match it explicitly and propagate the error; also propagate flush failures.Proposed fix
- while let Some(chunk) = chunks.next().await { - if let LlmResponseChunk::TextDelta { text, .. } = chunk? { - print!("{text}"); - std::io::stdout().flush().ok(); - } + while let Some(chunk) = chunks.next().await { + match chunk? { + LlmResponseChunk::TextDelta { text, .. } => { + print!("{text}"); + std::io::stdout().flush()?; + } + LlmResponseChunk::Error { message } => return Err(message.into()), + _ => {} + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.while let Some(chunk) = chunks.next().await { match chunk? { LlmResponseChunk::TextDelta { text, .. } => { print!("{text}"); std::io::stdout().flush()?; } LlmResponseChunk::Error { message } => return Err(message.into()), _ => {} } }🤖 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-examples/examples/streaming_agent.rs` around lines 88 - 92, Update the chunk handling loop in streaming_agent so LlmResponseChunk::Error is matched explicitly and propagated instead of being ignored, while preserving text output handling. Also replace the ignored stdout flush result with propagation of its failure from the surrounding async function.crates/libsy-examples/src/llm_class.rs (2)
114-120: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Synthetic requests retain stale provider payloads. If a routed client honors
raw_request, it bypasses the newly generated normalized prompt.
crates/libsy-examples/src/llm_class.rs#L114-L120: setraw_request: Noneor regenerate it with the classifier preamble.crates/libsy-examples/src/ensemble.rs#L273-L277: setraw_request: Noneor regenerate it with the judge prompt.📍 Affects 2 files
crates/libsy-examples/src/llm_class.rs#L114-L120(this comment)crates/libsy-examples/src/ensemble.rs#L273-L277🤖 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-examples/src/llm_class.rs` around lines 114 - 120, Clear stale raw_request payloads when constructing the synthetic classifier request in llm_class.rs around lines 114-120, using None or regenerating it with the classifier preamble. Apply the same change to the synthetic judge request in ensemble.rs around lines 273-277, ensuring both routed requests use their newly generated prompts instead of inherited provider payloads.
137-144: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The algorithms only inspect buffered responses despite accepting streaming routed clients. Calling
agg()silently maps streams toNone, changing routing decisions.
crates/libsy-examples/src/llm_class.rs#L137-L144: callaggregate().await?before parsing the classifier score.crates/libsy-examples/src/ensemble.rs#L278-L289: aggregate the judge response before parsing its selection.crates/libsy-examples/src/ensemble.rs#L334-L342: aggregate candidate responses before building the judge prompt, retaining the resulting aggregate as the potential winner response.📍 Affects 2 files
crates/libsy-examples/src/llm_class.rs#L137-L144(this comment)crates/libsy-examples/src/ensemble.rs#L278-L289crates/libsy-examples/src/ensemble.rs#L334-L342🤖 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-examples/src/llm_class.rs` around lines 137 - 144, The response-processing algorithms incorrectly use agg(), which ignores streaming responses. In crates/libsy-examples/src/llm_class.rs lines 137-144, replace this with aggregate().await? before parsing the classifier score; in crates/libsy-examples/src/ensemble.rs lines 278-289, aggregate the judge response before parsing its selection; and in crates/libsy-examples/src/ensemble.rs lines 334-342, aggregate candidate responses before constructing the judge prompt and retain each aggregate as the potential winner response.crates/libsy-protocol/src/stream.rs (2)
151-173: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve streamed content-block index and order.
The accumulator discards text/reasoning indices and always emits reasoning, then merged text, then tool calls. A stream such as text → tool call → text is therefore reordered and cannot round-trip through provider encoders. Store partial content by index and emit it in index order.
Also applies to: 182-200
🤖 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-protocol/src/stream.rs` around lines 151 - 173, Update the response accumulator around the LlmResponseChunk handling and its emission logic to retain each text, reasoning, and tool-call content block under its streamed index, rather than merging by type. Emit the accumulated blocks in ascending index order so interleaved sequences such as text → tool call → text preserve their original order and can round-trip through provider encoders.
175-177: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve an earlier stop reason when
MessageStophas none.A reasonless terminal event currently overwrites
MaxTokensorToolUsewithEndTurn. This contradictscrates/switchyard-translation/tests/stream_translation.rs’s Anthropic stop-reason contract.Proposed fix
LlmResponseChunk::MessageStop { reason } => { - self.stop_reason = Some(stop_reason_from_str(reason.as_deref())); + if reason.is_some() || self.stop_reason.is_none() { + self.stop_reason = Some(stop_reason_from_str(reason.as_deref())); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.LlmResponseChunk::MessageStop { reason } => { if reason.is_some() || self.stop_reason.is_none() { self.stop_reason = Some(stop_reason_from_str(reason.as_deref())); } }🤖 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-protocol/src/stream.rs` around lines 175 - 177, Update the LlmResponseChunk::MessageStop handling to only assign stop_reason when the event provides a reason; preserve any previously stored MaxTokens or ToolUse value when reason is absent, while retaining the existing EndTurn mapping for explicit reasons.crates/libsy/README.md (2)
82-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the routed-client example self-contained.
The snippet uses
Decision,Response,LlmResponse, andtext_response, but the documented imports do not provide them. It also usesasync_trait::async_traitwithout stating that the consumer must addasync-traitas a direct dependency. As written, copying this example will not compile.🤖 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/README.md` around lines 82 - 100, The RoutedLlmClient example in README.md is not self-contained because its required types, helper, and async-trait dependency are unspecified. Add the necessary imports for Decision, Response, LlmResponse, and text_response from their actual modules, and document that consumers must declare async-trait as a direct dependency before using the async_trait attribute.
237-239: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant README section git ls-files crates/libsy/README.md nl -ba crates/libsy/README.md | sed -n '210,260p' # Find the aggregate API and any docs/comments around its error behavior rg -n "aggregate\(\)|mid-stream error|surfaces errors|completion_text\(" crates -g '!**/target/**' # Map relevant source files before reading them fd -a ".*" crates | rg "llm_response|aggregate|completion_text|response|README.md"Repository: NVIDIA-NeMo/Switchyard
Length of output: 223
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- README excerpt ---' sed -n '210,260p' crates/libsy/README.md | cat -n echo echo '--- aggregate / completion_text references ---' rg -n "aggregate\(\)|mid-stream error|surfaces errors|completion_text\(" crates echo echo '--- candidate files ---' fd -a "aggregate|completion_text|llm_response|README.md" cratesRepository: NVIDIA-NeMo/Switchyard
Length of output: 5596
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- libsy aggregate docs/implementation ---' sed -n '150,240p' crates/libsy/src/lib.rs | cat -n echo echo '--- aggregate call sites in libsy source ---' sed -n '580,640p' crates/libsy/src/lib.rs | cat -n echo echo '--- protocol completion_text / response types ---' sed -n '1,180p' crates/libsy-protocol/src/lib.rs | cat -nRepository: NVIDIA-NeMo/Switchyard
Length of output: 12395
Handle
aggregate()errors explicitly..ok()turns a mid-stream failure intoNone, so this example treats upstream stream errors the same as “no score” and routes to the strong model. If fail-open is intended, match onErrexplicitly; otherwise propagate the error with?.🤖 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/README.md` around lines 237 - 239, Update the score extraction around classify_response.llm_response.aggregate().await to handle aggregation errors explicitly instead of silently converting them with .ok(). Match the Err case according to the intended fail-open behavior, or propagate it with ?; preserve the existing parsing and no-score handling for successful aggregates.crates/switchyard-translation/src/codecs/anthropic/stream.rs (1)
89-109: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Emit
MessageStoponly for terminalmessage_stop.Line 98 emits a terminal chunk during
message_delta, then Lines 107-109 emit it again. Remember the reason atmessage_delta, but emit the soleMessageStopwhen the terminal provider event arrives; otherwise direct stream consumers can process completion twice.Proposed fix
{ // Remember the provider stop reason: Anthropic delivers it here, on // `message_delta`, while the terminal `message_stop` carries none of its own. state.stop_reason = Some(stop_reason.to_string()); - out.push(LlmResponseChunk::MessageStop { - reason: Some(stop_reason.to_string()), - }); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if let Some(stop_reason) = object .get("delta") .and_then(Value::as_object) .and_then(|delta| delta.get("stop_reason")) .and_then(Value::as_str) { // Remember the provider stop reason: Anthropic delivers it here, on // `message_delta`, while the terminal `message_stop` carries none of its own. state.stop_reason = Some(stop_reason.to_string()); } out } // Anthropic's terminal `message_stop` carries no reason of its own; replay the one // remembered from `message_delta` so a chunk accumulator keeps the real stop reason // (e.g. `max_tokens`) instead of overwriting it with a reasonless `EndTurn`. Some("message_stop") => vec![LlmResponseChunk::MessageStop { reason: state.stop_reason.clone(), }],🤖 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/codecs/anthropic/stream.rs` around lines 89 - 109, Update the `message_delta` handling to only store `stop_reason` in `state.stop_reason` without pushing `LlmResponseChunk::MessageStop`. Keep the single `MessageStop` emission in the `message_stop` branch, using the remembered reason so direct consumers receive completion exactly once.
messiaen
force-pushed
the
grclark/libsy-streaming-support
branch
from
July 16, 2026 17:58
6547267 to
1a0b14c
Compare
messiaen
force-pushed
the
grclark/libsy-lib-proto-cleanup
branch
from
July 16, 2026 18:30
399a407 to
9e31bda
Compare
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/libsy-examples/src/ensemble.rs (2)
188-195: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve the full inbound
LlmRequest.
text_request(prompt_text(...))drops message roles, instructions, tools, sampling/output settings, and provider extensions. Forward or clone the original request for routed candidate/committed calls.
crates/libsy-examples/src/ensemble.rs#L188-L195: send the original committed request unchanged.crates/libsy-examples/src/ensemble.rs#L228-L232: clone the original request per candidate rather than rebuilding text-only requests.🤖 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-examples/src/ensemble.rs` around lines 188 - 195, Preserve the complete inbound LlmRequest instead of rebuilding text-only requests with text_request and prompt_text. In crates/libsy-examples/src/ensemble.rs lines 188-195, make the routed committed call send the original request unchanged; in lines 228-232, clone the original request for each candidate while applying only the candidate-specific routing changes required by the existing flow.
282-287: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAggregate streamed responses before judging.
RoutedLlmClientmay returnLlmResponse::Stream, butas_agg()yieldsNonefor streams. Candidate prompts become empty and streamed judge output silently falls back to candidate zero. Aggregate responses withinto_agg().awaitbefore constructing/parsing the judge exchange.
crates/libsy-examples/src/ensemble.rs#L282-L287: aggregate the judge response beforeparse_choice.crates/libsy-examples/src/ensemble.rs#L337-L341: aggregate candidate responses before embedding them in the judge prompt.🤖 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-examples/src/ensemble.rs` around lines 282 - 287, The judge response at crates/libsy-examples/src/ensemble.rs lines 282-287 must be aggregated before parse_choice; await into_agg() and parse the resulting aggregate text. Apply the same aggregation before embedding candidate responses in the judge prompt at crates/libsy-examples/src/ensemble.rs lines 337-341, preserving the existing prompt and choice-selection behavior.
🤖 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/libsy-protocol/src/envelope.rs`:
- Around line 10-16: Update the doc comment for the public Context type to
remove the inaccurate “empty today” description and document the intended
contract of Context::values, including that it stores per-request cross-cutting
state as string key/value entries. Keep the existing derives and field
unchanged.
---
Outside diff comments:
In `@crates/libsy-examples/src/ensemble.rs`:
- Around line 188-195: Preserve the complete inbound LlmRequest instead of
rebuilding text-only requests with text_request and prompt_text. In
crates/libsy-examples/src/ensemble.rs lines 188-195, make the routed committed
call send the original request unchanged; in lines 228-232, clone the original
request for each candidate while applying only the candidate-specific routing
changes required by the existing flow.
- Around line 282-287: The judge response at
crates/libsy-examples/src/ensemble.rs lines 282-287 must be aggregated before
parse_choice; await into_agg() and parse the resulting aggregate text. Apply the
same aggregation before embedding candidate responses in the judge prompt at
crates/libsy-examples/src/ensemble.rs lines 337-341, preserving the existing
prompt and choice-selection behavior.
🪄 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: 804fc475-2e8b-4b59-b5eb-e4fc3584c4fc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (10)
crates/libsy-examples/examples/research_agent.rscrates/libsy-examples/src/ensemble.rscrates/libsy-examples/src/llm_class.rscrates/libsy-examples/src/rand.rscrates/libsy-protocol/Cargo.tomlcrates/libsy-protocol/src/client.rscrates/libsy-protocol/src/envelope.rscrates/libsy-protocol/src/lib.rscrates/libsy/README.mdcrates/libsy/src/lib.rs
grahamking
approved these changes
Jul 16, 2026
messiaen
force-pushed
the
grclark/libsy-lib-proto-cleanup
branch
from
July 16, 2026 23:11
9e31bda to
f2fc719
Compare
Signed-off-by: Greg Clark <grclark@nvidia.com>
messiaen
force-pushed
the
grclark/libsy-lib-proto-cleanup
branch
from
July 16, 2026 23:13
f2fc719 to
57c015d
Compare
messiaen
enabled auto-merge (squash)
July 16, 2026 23:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cleans up the
libsy/switchyard-protocolboundary on top of the streaming work:moves the routed-call vocabulary into the protocol crate and threads request context
through the call path.
Base:
libsy-streaming-supportChanges
switchyard-protocolclientmodule owns theDecisionandRoutedLlmClienttraits — so a crate thatdepends only on the protocol can serve routed calls without pulling in the
libsyorchestrator.
Contextmoves here (and carries avaluesmap);Metadatagains awire_formatfield.libsyLlmClient→RoutedLlmClient, re-exported from the protocol crate along withDecision;common.rsremoved (itsContextnow lives in the protocol).RoutedRequestcarriesctx, andRoutedLlmClient::calltakes(ctx, request, decision).Surrounding
libsy-examples(random, classifier, ensemble routers +research_agent) updated for therenamed trait and ctx threading.
Validation
Full workspace builds clean, clippy at zero, all tests pass.
uv run ruff check .cleanuv run mypy switchyardcleanuv run pytest tests/greenChecklist
snake_caseof the primary class.switchyard/__init__.py.__all__if intended for downstream use.--helpupdated if customer-facing surface changed.Signed-off-by: Your Name <email>) per the DCO.Notes for reviewers
Anything reviewers should pay extra attention to — risky paths, follow-up tickets, intentional trade-offs.
Summary by CodeRabbit