fix: improve OpenRouter protocol interoperability - #237
Open
nachiketb-nvidia wants to merge 1 commit into
Open
Conversation
Signed-off-by: nachiketb <nachiketb@nvidia.com>
WalkthroughThe client reserves ChangesHeader forwarding
Stream error metrics
Stream translation
Estimated code review effort: 3 (Moderate) | ~20 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: 2
🧹 Nitpick comments (1)
crates/switchyard-server/tests/server.rs (1)
1133-1133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the important stream-failure contract.
Add a short comment stating that a terminal stream failure increments error statistics and metrics without recording usage or terminal latency.
As per coding guidelines, “comments for ... important tests” are required.
Proposed comment
+// A terminal stream failure records errors without usage or terminal latency. async fn streaming_error_records_error_without_usage_or_latency() -> TestResult {🤖 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-server/tests/server.rs` at line 1133, Above the streaming_error_records_error_without_usage_or_latency test, add a brief comment documenting that terminal stream failures increment error statistics and metrics while recording neither usage nor terminal latency.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/switchyard-server/src/usage_metrics.rs`:
- Around line 94-102: Normalize the optional tier value at the start of
record_stream_error, trimming whitespace and converting empty values to None,
then pass the normalized tier to both StatsAccumulator::record_stream_error and
attributes. Apply the same normalization consistently in the related token and
latency metric recording paths that currently pass the original tier.
In `@crates/switchyard-translation/src/sse.rs`:
- Around line 17-22: Update done_marker and the decode_stream flow so data:
[DONE] produces a distinct terminal result instead of being treated as an empty
frame. Detect the marker in parse_json_sse_frame or its caller, return the
marker-specific result, and break decoding immediately while preserving
empty-frame skipping and normal frame processing.
---
Nitpick comments:
In `@crates/switchyard-server/tests/server.rs`:
- Line 1133: Above the streaming_error_records_error_without_usage_or_latency
test, add a brief comment documenting that terminal stream failures increment
error statistics and metrics while recording neither usage nor terminal latency.
🪄 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: 036c77bf-f046-4ebe-b59d-f6648bdfd457
📒 Files selected for processing (7)
crates/libsy-llm-client/src/client.rscrates/switchyard-server/src/stats/accumulator.rscrates/switchyard-server/src/usage_metrics.rscrates/switchyard-server/tests/server.rscrates/switchyard-translation/src/codecs/openai_chat/stream.rscrates/switchyard-translation/src/sse.rscrates/switchyard-translation/tests/stream_translation.rs
Comment on lines
+94
to
+102
| // Records a terminal stream failure after the routed call was already counted. | ||
| fn record_stream_error(stats: &StatsAccumulator, model: &str, tier: Option<&str>) { | ||
| stats.record_stream_error(model, tier); | ||
| global::meter("switchyard") | ||
| .u64_counter("switchyard.errors") | ||
| .build() | ||
| .add(1, &attributes(model, tier)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 'fn normalized_tier|normalized_tier\(' \
crates/switchyard-server/src/stats \
crates/switchyard-server/src/usage_metrics.rs
rg -n -C 6 'record_stream_error|fn attributes|attributes\(' \
crates/switchyard-server/src/usage_metrics.rs \
crates/switchyard-server/src/statsRepository: NVIDIA-NeMo/Switchyard
Length of output: 11508
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- usage_metrics.rs ---'
cat -n crates/switchyard-server/src/usage_metrics.rs | sed -n '1,210p'
printf '%s\n' '--- tier-related call sites ---'
rg -n -C 4 'record_stream_error|record_usage|record_latency|tier\s*[:=]|tier\.as_deref|observe' \
crates/switchyard-server/src --glob '*.rs'Repository: NVIDIA-NeMo/Switchyard
Length of output: 38461
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- routing_tier definitions and implementations ---'
rg -n -C 8 'fn routing_tier|routing_tier\(|struct .*Decision|enum .*Decision|tier:' \
crates --glob '*.rs'
printf '%s\n' '--- tier normalization tests and related assertions ---'
rg -n -C 6 'normalized_tier|whitespace|trim\(\)|tier.*empty|Some\("\s' \
crates/switchyard-server --glob '*.rs'Repository: NVIDIA-NeMo/Switchyard
Length of output: 50379
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- production Decision implementations that override routing_tier ---'
rg -n -C 6 'impl\s+Decision\s+for|fn\s+routing_tier\s*\(' \
crates/switchyard-server crates/libsy crates/switchyard-components crates/protocol \
--glob '*.rs' | sed -n '1,260p'
printf '%s\n' '--- behavioral probe for normalized_tier ---'
python3 - <<'PY'
def normalized_tier(tier):
if tier is None:
return None
tier = tier.strip()
return tier if tier else None
for value in (None, "", " ", " capable ", "capable", "capable\n"):
print(repr(value), "=>", repr(normalized_tier(value)))
PYRepository: NVIDIA-NeMo/Switchyard
Length of output: 16227
Normalize tier before recording metrics.
StatsAccumulator trims tier and removes empty values, but attributes uses the original value. This affects switchyard.errors, token metrics, and latency metrics. Normalize tier before passing it to both statistics and metric recording.
🤖 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-server/src/usage_metrics.rs` around lines 94 - 102,
Normalize the optional tier value at the start of record_stream_error, trimming
whitespace and converting empty values to None, then pass the normalized tier to
both StatsAccumulator::record_stream_error and attributes. Apply the same
normalization consistently in the related token and latency metric recording
paths that currently pass the original tier.
Comment on lines
+17
to
+22
| /// The optional terminal SSE marker accepted for all supported wire formats. | ||
| /// Native Anthropic streams can end at `message_stop`, while compatible | ||
| /// providers may additionally send `[DONE]`. | ||
| #[inline] | ||
| pub(crate) fn done_marker(format: WireFormat) -> Option<&'static str> { | ||
| match format { | ||
| WireFormat::OpenAiChat | WireFormat::OpenAiResponses => Some("[DONE]"), | ||
| WireFormat::AnthropicMessages => None, | ||
| } | ||
| pub(crate) fn done_marker(_format: WireFormat) -> Option<&'static str> { | ||
| Some("[DONE]") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Expect a marker-specific branch that exits the decode loop.
rg -n -C 16 \
'fn decode_stream|parse_json_sse_frame|while let Some\(line\)|done_marker' \
crates/switchyard-translation/srcRepository: NVIDIA-NeMo/Switchyard
Length of output: 27886
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect all marker/parser call sites and stream tests.
rg -n -C 8 \
'parse_json_sse_frame|done_marker|decode_stream\(|decode_all|data: \[DONE\]' \
crates/switchyard-translation/src
# Deterministically model the current control flow for a terminal marker followed
# by another valid frame. This does not execute repository code.
python3 - <<'PY'
from pathlib import Path
helpers = Path("crates/switchyard-translation/src/helpers.rs").read_text()
sse = Path("crates/switchyard-translation/src/sse.rs").read_text()
required = [
"let marker = sse::done_marker(source);",
"if let Some(value) = sse::parse_json_sse_frame(&frame, marker)",
"frame.clear();",
"while let Some(line) = lines.next().await",
]
for text in required:
assert text in helpers, f"missing expected helper control-flow text: {text}"
assert "if data.is_empty() || done_marker.is_some_and(|marker| data == marker)" in sse
assert "return Ok(None);" in sse
# Equivalent abstraction of the observed loop:
# None is skipped, frame state is cleared, and the loop reads the next frame.
frames = ["data: {\"text\":\"before\"}", "data: [DONE]", "data: {\"text\":\"after\"}"]
decoded = []
for frame in frames:
data = frame.removeprefix("data: ").rstrip()
if not data or data == "[DONE]":
continue
decoded.append(data)
assert decoded == ['{"text":"before"}', '{"text":"after"}']
print("Current parser/decoder shape: [DONE] yields None, None is skipped, and a later frame is decoded.")
PYRepository: NVIDIA-NeMo/Switchyard
Length of output: 22361
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 'parse_json_sse_frame|done_marker|decode_stream\(|decode_all|data: \[DONE\]' crates/switchyard-translation/src
python3 - <<'PY'
from pathlib import Path
helpers = Path("crates/switchyard-translation/src/helpers.rs").read_text()
sse = Path("crates/switchyard-translation/src/sse.rs").read_text()
assert "return Ok(None);" in sse
assert "if let Some(value) = sse::parse_json_sse_frame(&frame, marker)" in helpers
frames = ["data: {\"text\":\"before\"}", "data: [DONE]", "data: {\"text\":\"after\"}"]
decoded = []
for frame in frames:
data = frame.removeprefix("data: ").rstrip()
if not data or data == "[DONE]":
continue
decoded.append(data)
assert decoded == ['{"text":"before"}', '{"text":"after"}']
print("Current parser/decoder shape: [DONE] yields None, None is skipped, and a later frame is decoded.")
PYRepository: NVIDIA-NeMo/Switchyard
Length of output: 22361
Stop decoding when data: [DONE] is received.
parse_json_sse_frame returns None for both empty frames and [DONE]. decode_stream skips None and continues reading. If the upstream keeps the connection open, decoding waits for EOF and accepts frames after the terminal marker. Return a marker-specific result and break the loop.
🤖 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/sse.rs` around lines 17 - 22, Update
done_marker and the decode_stream flow so data: [DONE] produces a distinct
terminal result instead of being treated as an empty frame. Detect the marker in
parse_json_sse_frame or its caller, return the marker-specific result, and break
decoding immediately while preserving empty-frame skipping and normal frame
processing.
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
1 participant
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.
What
[DONE]termination on Anthropic-compatible SSE streamsWhy
OpenRouter exercises provider-compatible behavior that exposed four gaps: compressed responses the HTTP client could not decode, Anthropic streams ending with
[DONE], usage arriving afterfinish_reason, and stream failures being reported as successful in stats.How
The fixes stay at their existing ownership boundaries in the LLM client, translation codecs, and server usage observer. Routed calls remain counted once; terminal stream failures increment only the missing error counters.
What to review
Accept-EncodingValidation
cargo test -p switchyard-llm-client forwards_metadata_headers_except_reservedcargo test -p switchyard-translationcargo test -p switchyard-server streaming_error_records_error_without_usage_or_latencycargo clippy -p switchyard-llm-client -p switchyard-translation -p switchyard-server --all-targets -- -D warnings/v1/statsSummary by CodeRabbit
Accept-Encodingheaders are no longer forwarded unexpectedly.[DONE]marker across supported formats.