fix: resolve open OSS QA bugs across libsy, v2 server, CLI, and docs#103
Closed
elyasmnvidian wants to merge 9 commits into
Closed
fix: resolve open OSS QA bugs across libsy, v2 server, CLI, and docs#103elyasmnvidian wants to merge 9 commits into
elyasmnvidian wants to merge 9 commits into
Conversation
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
… overflow Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
…, clarify serve errors Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
… in --dry-run Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
…API references Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
|
WalkthroughThe PR updates profile-based routing, classifier streaming, context-error handling, model capabilities, CLI provider and launcher behavior, loopback proxy checks, no-op statistics, package metadata, examples, and related documentation and tests. ChangesRouting and serving behavior
CLI configuration and execution
API migration and documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
crates/switchyard-server/src/lib.rs (1)
549-589: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep the context-window fragments in sync with
switchyard/lib/model_listing.py— the table is duplicated here, and the first-match order means future model additions can drift silently. A small parity test would help.🤖 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/lib.rs` around lines 549 - 589, Keep inferred_context_window’s TABLE synchronized with the fragment mappings and precedence defined in switchyard/lib/model_listing.py, including future model additions and their first-match order. Add a focused parity test that verifies the Rust inference returns the same context window as the Python model_listing table for representative matching and fallback model names.
🤖 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/src/algorithms/llm_class.rs`:
- Around line 414-458: Remove the parenthetical NvBug tracker references from
the comments in out_of_range_score_defaults_to_strong and
streamed_score_below_threshold_routes_weak, while preserving the behavioral and
edge-case explanations in both comments.
In `@crates/switchyard-components-v2/src/profiles/random_routing.rs`:
- Around line 235-291: Update RandomRoutingProfile::run and
fallback_processed_request so a context-window overflow does not retry the same
target: when the selected target already equals fallback_target_on_evict, skip
the duplicate call and return ContextPoolExhausted with that target. Record the
first attempt’s ContextWindowExceeded via record_error before handling the
fallback, while preserving existing weak-to-strong retry behavior. Extend run’s
tests to cover overflow when the initially selected target is already the
fallback target.
In `@INSTALLATION.md`:
- Line 70: The documentation uses inconsistent extra naming. In
INSTALLATION.md:70-70 and README.md:219-219, update the documented extra name to
affinity-redis to match pyproject.toml, replacing any redis-affinity wording.
In `@switchyard/cli/configure_command.py`:
- Around line 425-437: Update the configure flow around
resolve_provider_connectivity() to retain its resolved provider alongside
env_api_key, and use that provider when persisting the selected configuration
and credentials. Ensure an environment key cannot be saved under a different
provider while retaining the selected provider’s base URL; alternatively
restrict environment-key resolution to the selected provider.
In `@switchyard/cli/launch_command.py`:
- Line 127: Update the endpoint check in the launch command around
_OPENROUTER_URL_MARKER to parse base_url and compare its normalized hostname
against the intended OpenRouter host, rather than checking for a substring.
Ensure case-insensitive matching and reject unrelated hosts such as
notopenrouter.ai so the fail-fast guard remains effective.
In `@switchyard/cli/switchyard_cli.py`:
- Around line 1463-1470: Update the subcommand detection around _SUBCOMMANDS,
sep_idx, and cmd_idx so option values are not mistaken for the command. Identify
the subcommand only from the structurally valid global-option prefix, or strip
the separator only when its following token is a recognized subcommand; preserve
separators intended for forwarding after the actual command.
In `@tests/test_launch_codex.py`:
- Around line 509-511: Remove the “NvBug 6401771” tracker reference from the
source comment near the OpenRouter endpoint logic, while preserving the
explanation that non-OpenRouter providers are rejected.
In `@tests/test_user_config.py`:
- Around line 361-368: Replace function-stubbing monkeypatch.setattr calls with
mocker.patch while preserving each stub’s behavior; retain monkeypatch only for
environment variables. Apply this in tests/test_user_config.py ranges 361-368
and 390-398, tests/test_launch_claude_deterministic.py ranges 271-275, 292-295,
and 319-333, and tests/test_launch_codex.py range 512-515.
---
Nitpick comments:
In `@crates/switchyard-server/src/lib.rs`:
- Around line 549-589: Keep inferred_context_window’s TABLE synchronized with
the fragment mappings and precedence defined in switchyard/lib/model_listing.py,
including future model additions and their first-match order. Add a focused
parity test that verifies the Rust inference returns the same context window as
the Python model_listing table for representative matching and fallback model
names.
🪄 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: 497ad048-3dd5-49fd-95a2-a26aa323a82b
📒 Files selected for processing (31)
CHANGELOG.mdDEVELOPMENT.mdINSTALLATION.mdREADME.mdcrates/libsy/Cargo.tomlcrates/libsy/src/algorithms/llm_class.rscrates/libsy/src/core/algorithm.rscrates/libsy/src/lib.rscrates/switchyard-components-v2/src/profiles/random_routing.rscrates/switchyard-components-v2/tests/random_routing_profile.rscrates/switchyard-server/src/lib.rscrates/switchyard-server/tests/server.rsdocs/cli_reference.mddocs/guides/agent_launchers.mdexamples/minimal.pyexamples/route.yamlswitchyard/cli/configure_command.pyswitchyard/cli/launch_command.pyswitchyard/cli/launchers/launcher_runtime.pyswitchyard/cli/launchers/proxy_health_monitor.pyswitchyard/cli/output.pyswitchyard/cli/switchyard_cli.pyswitchyard/lib/profiles/noop.pyswitchyard/server/verify.pytests/test_launch_claude.pytests/test_launch_claude_deterministic.pytests/test_launch_codex.pytests/test_launcher_proxy_bypass.pytests/test_noop_llm_backend.pytests/test_serve_profile_config.pytests/test_user_config.py
Comment on lines
+414
to
+458
|
|
||
| #[tokio::test] | ||
| async fn out_of_range_score_defaults_to_strong() -> Result<(), Box<dyn Error + Send + Sync>> { | ||
| // "NaN" parses as an f64 but is not a usable probability, so it must fail | ||
| // open to strong rather than be treated as a below-threshold verdict and | ||
| // silently downgraded to weak (NvBug 6485976). | ||
| let (algo, _) = algo(0.5, "NaN"); | ||
| let (trace, response) = orch(algo).run(Context::default(), request("hi")).await?; | ||
| assert_eq!( | ||
| response | ||
| .llm_response | ||
| .as_agg() | ||
| .map(completion_text) | ||
| .unwrap_or_default(), | ||
| "answer from frontier/model" | ||
| ); | ||
| let routed = as_classifier(&trace[1])?; | ||
| assert_eq!(routed.tier, Some(ClassifierTier::Strong)); | ||
| assert_eq!(routed.score, None); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn streamed_score_below_threshold_routes_weak() -> Result<(), Box<dyn Error + Send + Sync>> | ||
| { | ||
| // A streaming classifier target must have its score drained and read, not | ||
| // dropped — otherwise every streamed verdict falls open to strong | ||
| // regardless of value (NvBug 6485975). | ||
| let (algo, _) = algo_streaming(0.5, "0.2"); | ||
| let (trace, response) = orch(algo) | ||
| .run(Context::default(), request("say hello")) | ||
| .await?; | ||
| assert_eq!( | ||
| response | ||
| .llm_response | ||
| .as_agg() | ||
| .map(completion_text) | ||
| .unwrap_or_default(), | ||
| "answer from cheap/model" | ||
| ); | ||
| let routed = as_classifier(&trace[1])?; | ||
| assert_eq!(routed.tier, Some(ClassifierTier::Weak)); | ||
| assert_eq!(routed.score, Some(0.2)); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Drop the bug-tracker IDs from test comments.
Both new test doc-comments end with a parenthetical tracker reference — "(NvBug 6485976)" (line ~419) and "(NvBug 6485975)" (line ~441). Per the coding guideline for **/*.{py,rs}, source comments should explain behavior/invariants, not carry project-management references like issue IDs.
As per coding guidelines, "Source comments must explain code behavior, invariants, or non-obvious edge cases; do not include project-management references such as tracker steps, issue IDs, or plan links."
📝 Proposed fix
- // "NaN" parses as an f64 but is not a usable probability, so it must fail
- // open to strong rather than be treated as a below-threshold verdict and
- // silently downgraded to weak (NvBug 6485976).
+ // "NaN" parses as an f64 but is not a usable probability, so it must fail
+ // open to strong rather than be treated as a below-threshold verdict and
+ // silently downgraded to weak.- // A streaming classifier target must have its score drained and read, not
- // dropped — otherwise every streamed verdict falls open to strong
- // regardless of value (NvBug 6485975).
+ // A streaming classifier target must have its score drained and read, not
+ // dropped — otherwise every streamed verdict falls open to strong
+ // regardless of value.📝 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.
Suggested change
| #[tokio::test] | |
| async fn out_of_range_score_defaults_to_strong() -> Result<(), Box<dyn Error + Send + Sync>> { | |
| // "NaN" parses as an f64 but is not a usable probability, so it must fail | |
| // open to strong rather than be treated as a below-threshold verdict and | |
| // silently downgraded to weak (NvBug 6485976). | |
| let (algo, _) = algo(0.5, "NaN"); | |
| let (trace, response) = orch(algo).run(Context::default(), request("hi")).await?; | |
| assert_eq!( | |
| response | |
| .llm_response | |
| .as_agg() | |
| .map(completion_text) | |
| .unwrap_or_default(), | |
| "answer from frontier/model" | |
| ); | |
| let routed = as_classifier(&trace[1])?; | |
| assert_eq!(routed.tier, Some(ClassifierTier::Strong)); | |
| assert_eq!(routed.score, None); | |
| Ok(()) | |
| } | |
| #[tokio::test] | |
| async fn streamed_score_below_threshold_routes_weak() -> Result<(), Box<dyn Error + Send + Sync>> | |
| { | |
| // A streaming classifier target must have its score drained and read, not | |
| // dropped — otherwise every streamed verdict falls open to strong | |
| // regardless of value (NvBug 6485975). | |
| let (algo, _) = algo_streaming(0.5, "0.2"); | |
| let (trace, response) = orch(algo) | |
| .run(Context::default(), request("say hello")) | |
| .await?; | |
| assert_eq!( | |
| response | |
| .llm_response | |
| .as_agg() | |
| .map(completion_text) | |
| .unwrap_or_default(), | |
| "answer from cheap/model" | |
| ); | |
| let routed = as_classifier(&trace[1])?; | |
| assert_eq!(routed.tier, Some(ClassifierTier::Weak)); | |
| assert_eq!(routed.score, Some(0.2)); | |
| Ok(()) | |
| } | |
| #[tokio::test] | |
| async fn out_of_range_score_defaults_to_strong() -> Result<(), Box<dyn Error + Send + Sync>> { | |
| // "NaN" parses as an f64 but is not a usable probability, so it must fail | |
| // open to strong rather than be treated as a below-threshold verdict and | |
| // silently downgraded to weak. | |
| let (algo, _) = algo(0.5, "NaN"); | |
| let (trace, response) = orch(algo).run(Context::default(), request("hi")).await?; | |
| assert_eq!( | |
| response | |
| .llm_response | |
| .as_agg() | |
| .map(completion_text) | |
| .unwrap_or_default(), | |
| "answer from frontier/model" | |
| ); | |
| let routed = as_classifier(&trace[1])?; | |
| assert_eq!(routed.tier, Some(ClassifierTier::Strong)); | |
| assert_eq!(routed.score, None); | |
| Ok(()) | |
| } | |
| #[tokio::test] | |
| async fn streamed_score_below_threshold_routes_weak() -> Result<(), Box<dyn Error + Send + Sync>> | |
| { | |
| // A streaming classifier target must have its score drained and read, not | |
| // dropped — otherwise every streamed verdict falls open to strong | |
| // regardless of value. | |
| let (algo, _) = algo_streaming(0.5, "0.2"); | |
| let (trace, response) = orch(algo) | |
| .run(Context::default(), request("say hello")) | |
| .await?; | |
| assert_eq!( | |
| response | |
| .llm_response | |
| .as_agg() | |
| .map(completion_text) | |
| .unwrap_or_default(), | |
| "answer from cheap/model" | |
| ); | |
| let routed = as_classifier(&trace[1])?; | |
| assert_eq!(routed.tier, Some(ClassifierTier::Weak)); | |
| assert_eq!(routed.score, Some(0.2)); | |
| Ok(()) | |
| } |
🤖 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/algorithms/llm_class.rs` around lines 414 - 458, Remove the
parenthetical NvBug tracker references from the comments in
out_of_range_score_defaults_to_strong and
streamed_score_below_threshold_routes_weak, while preserving the behavioral and
edge-case explanations in both comments.
Source: Coding guidelines
Comment on lines
235
to
291
| impl Profile for RandomRoutingProfile { | ||
| /// Executes random routing while keeping selected-target state local to this call. | ||
| /// Executes random routing with one context-window fallback retry. | ||
| async fn run(&self, input: ProfileInput) -> Result<ProfileResponse> { | ||
| let profile_started_at = Instant::now(); | ||
| let processed = self.process(input).await?; | ||
| let decision = &processed.decision; | ||
| let selected_backend = self.selected_backend(decision)?; | ||
| let backend_started_at = Instant::now(); | ||
| let response = match selected_backend | ||
| .call(&processed.profile_input.request) | ||
| .await | ||
| { | ||
| Ok(response) => response, | ||
| Err(error) => { | ||
| self.stats.record_error( | ||
| decision.selected_model.as_str(), | ||
| Some(decision.tier.as_str()), | ||
| let (first_result, first_backend_latency_ms) = self.call_selected(&processed).await; | ||
| match first_result { | ||
| Ok(response) => { | ||
| let response = self.record_success( | ||
| &processed.decision, | ||
| response, | ||
| profile_started_at, | ||
| first_backend_latency_ms, | ||
| )?; | ||
| return Err(error); | ||
| let response = self.rprocess(&processed, response).await?; | ||
| Ok(ProfileResponse::with_routing_metadata( | ||
| response, | ||
| self.routing_metadata(&processed.decision), | ||
| )) | ||
| } | ||
| }; | ||
| let backend_latency_ms = backend_started_at.elapsed().as_secs_f64() * 1000.0; | ||
| self.stats.record_success( | ||
| decision.selected_model.as_str(), | ||
| Some(backend_latency_ms), | ||
| Some(decision.tier.as_str()), | ||
| )?; | ||
| let response = record_usage_or_wrap_stream( | ||
| &self.stats, | ||
| decision.selected_model.as_str(), | ||
| Some(decision.tier.as_str()), | ||
| profile_started_at, | ||
| backend_latency_ms, | ||
| response, | ||
| )?; | ||
| let response = self.rprocess(&processed, response).await?; | ||
| Ok(ProfileResponse::with_routing_metadata( | ||
| response, | ||
| self.routing_metadata(decision), | ||
| )) | ||
| Err(SwitchyardError::ContextWindowExceeded { .. }) => { | ||
| let retry = self.fallback_processed_request(&processed)?; | ||
| let (retry_result, retry_backend_latency_ms) = self.call_selected(&retry).await; | ||
| match retry_result { | ||
| Ok(response) => { | ||
| let response = self.record_success( | ||
| &retry.decision, | ||
| response, | ||
| profile_started_at, | ||
| retry_backend_latency_ms, | ||
| )?; | ||
| let response = self.rprocess(&retry, response).await?; | ||
| Ok(ProfileResponse::with_routing_metadata( | ||
| response, | ||
| self.routing_metadata(&retry.decision), | ||
| )) | ||
| } | ||
| Err(SwitchyardError::ContextWindowExceeded { target_id, .. }) => { | ||
| self.record_error(&retry.decision)?; | ||
| Err(SwitchyardError::ContextPoolExhausted { | ||
| last_target_id: target_id, | ||
| reason: "all attempted targets returned context-window overflow" | ||
| .to_string(), | ||
| }) | ||
| } | ||
| Err(error) => { | ||
| self.record_error(&retry.decision)?; | ||
| Err(error) | ||
| } | ||
| } | ||
| } | ||
| Err(error) => { | ||
| self.record_error(&processed.decision)?; | ||
| Err(error) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fallback retry can dispatch to the same target that just overflowed.
fallback_processed_request always rewrites the retry to self.fallback_target_on_evict, without checking whether the originally selected target is already that fallback target. Since the default fallback is the strong target, any overflow on the strong target (the common high-probability path) triggers a second, identical call to the same backend — guaranteed to overflow again — before finally returning ContextPoolExhausted. That's a wasted upstream call (latency + cost) with no chance of success.
Two related gaps in the same flow:
- The first attempt's
ContextWindowExceededis never recorded viarecord_errorwhen the retry succeeds, so stats never reflect that a fallback occurred. - The new test (
run_retries_fallback_target_after_context_overflow, lines 696-759) only covers weak→strong fallback; it doesn't cover the case where the originally selected target already equalsfallback_target_on_evict.
🐛 Proposed fix
Err(SwitchyardError::ContextWindowExceeded { .. }) => {
+ if processed.decision.selected_target == self.fallback_target_on_evict {
+ // Already tried the only configured fallback target; a retry
+ // would just repeat the identical failing call.
+ self.record_error(&processed.decision)?;
+ return Err(SwitchyardError::ContextPoolExhausted {
+ last_target_id: processed.decision.selected_target.clone(),
+ reason: "all attempted targets returned context-window overflow"
+ .to_string(),
+ });
+ }
let retry = self.fallback_processed_request(&processed)?;📝 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.
Suggested change
| impl Profile for RandomRoutingProfile { | |
| /// Executes random routing while keeping selected-target state local to this call. | |
| /// Executes random routing with one context-window fallback retry. | |
| async fn run(&self, input: ProfileInput) -> Result<ProfileResponse> { | |
| let profile_started_at = Instant::now(); | |
| let processed = self.process(input).await?; | |
| let decision = &processed.decision; | |
| let selected_backend = self.selected_backend(decision)?; | |
| let backend_started_at = Instant::now(); | |
| let response = match selected_backend | |
| .call(&processed.profile_input.request) | |
| .await | |
| { | |
| Ok(response) => response, | |
| Err(error) => { | |
| self.stats.record_error( | |
| decision.selected_model.as_str(), | |
| Some(decision.tier.as_str()), | |
| let (first_result, first_backend_latency_ms) = self.call_selected(&processed).await; | |
| match first_result { | |
| Ok(response) => { | |
| let response = self.record_success( | |
| &processed.decision, | |
| response, | |
| profile_started_at, | |
| first_backend_latency_ms, | |
| )?; | |
| return Err(error); | |
| let response = self.rprocess(&processed, response).await?; | |
| Ok(ProfileResponse::with_routing_metadata( | |
| response, | |
| self.routing_metadata(&processed.decision), | |
| )) | |
| } | |
| }; | |
| let backend_latency_ms = backend_started_at.elapsed().as_secs_f64() * 1000.0; | |
| self.stats.record_success( | |
| decision.selected_model.as_str(), | |
| Some(backend_latency_ms), | |
| Some(decision.tier.as_str()), | |
| )?; | |
| let response = record_usage_or_wrap_stream( | |
| &self.stats, | |
| decision.selected_model.as_str(), | |
| Some(decision.tier.as_str()), | |
| profile_started_at, | |
| backend_latency_ms, | |
| response, | |
| )?; | |
| let response = self.rprocess(&processed, response).await?; | |
| Ok(ProfileResponse::with_routing_metadata( | |
| response, | |
| self.routing_metadata(decision), | |
| )) | |
| Err(SwitchyardError::ContextWindowExceeded { .. }) => { | |
| let retry = self.fallback_processed_request(&processed)?; | |
| let (retry_result, retry_backend_latency_ms) = self.call_selected(&retry).await; | |
| match retry_result { | |
| Ok(response) => { | |
| let response = self.record_success( | |
| &retry.decision, | |
| response, | |
| profile_started_at, | |
| retry_backend_latency_ms, | |
| )?; | |
| let response = self.rprocess(&retry, response).await?; | |
| Ok(ProfileResponse::with_routing_metadata( | |
| response, | |
| self.routing_metadata(&retry.decision), | |
| )) | |
| } | |
| Err(SwitchyardError::ContextWindowExceeded { target_id, .. }) => { | |
| self.record_error(&retry.decision)?; | |
| Err(SwitchyardError::ContextPoolExhausted { | |
| last_target_id: target_id, | |
| reason: "all attempted targets returned context-window overflow" | |
| .to_string(), | |
| }) | |
| } | |
| Err(error) => { | |
| self.record_error(&retry.decision)?; | |
| Err(error) | |
| } | |
| } | |
| } | |
| Err(error) => { | |
| self.record_error(&processed.decision)?; | |
| Err(error) | |
| } | |
| } | |
| } | |
| impl Profile for RandomRoutingProfile { | |
| /// Executes random routing with one context-window fallback retry. | |
| async fn run(&self, input: ProfileInput) -> Result<ProfileResponse> { | |
| let profile_started_at = Instant::now(); | |
| let processed = self.process(input).await?; | |
| let (first_result, first_backend_latency_ms) = self.call_selected(&processed).await; | |
| match first_result { | |
| Ok(response) => { | |
| let response = self.record_success( | |
| &processed.decision, | |
| response, | |
| profile_started_at, | |
| first_backend_latency_ms, | |
| )?; | |
| let response = self.rprocess(&processed, response).await?; | |
| Ok(ProfileResponse::with_routing_metadata( | |
| response, | |
| self.routing_metadata(&processed.decision), | |
| )) | |
| } | |
| Err(SwitchyardError::ContextWindowExceeded { .. }) => { | |
| if processed.decision.selected_target == self.fallback_target_on_evict { | |
| // Already tried the only configured fallback target; a retry | |
| // would just repeat the identical failing call. | |
| self.record_error(&processed.decision)?; | |
| return Err(SwitchyardError::ContextPoolExhausted { | |
| last_target_id: processed.decision.selected_target.clone(), | |
| reason: "all attempted targets returned context-window overflow" | |
| .to_string(), | |
| }); | |
| } | |
| let retry = self.fallback_processed_request(&processed)?; | |
| let (retry_result, retry_backend_latency_ms) = self.call_selected(&retry).await; | |
| match retry_result { | |
| Ok(response) => { | |
| let response = self.record_success( | |
| &retry.decision, | |
| response, | |
| profile_started_at, | |
| retry_backend_latency_ms, | |
| )?; | |
| let response = self.rprocess(&retry, response).await?; | |
| Ok(ProfileResponse::with_routing_metadata( | |
| response, | |
| self.routing_metadata(&retry.decision), | |
| )) | |
| } | |
| Err(SwitchyardError::ContextWindowExceeded { target_id, .. }) => { | |
| self.record_error(&retry.decision)?; | |
| Err(SwitchyardError::ContextPoolExhausted { | |
| last_target_id: target_id, | |
| reason: "all attempted targets returned context-window overflow" | |
| .to_string(), | |
| }) | |
| } | |
| Err(error) => { | |
| self.record_error(&retry.decision)?; | |
| Err(error) | |
| } | |
| } | |
| } | |
| Err(error) => { | |
| self.record_error(&processed.decision)?; | |
| Err(error) | |
| } | |
| } | |
| } |
🤖 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-components-v2/src/profiles/random_routing.rs` around lines
235 - 291, Update RandomRoutingProfile::run and fallback_processed_request so a
context-window overflow does not retry the same target: when the selected target
already equals fallback_target_on_evict, skip the duplicate call and return
ContextPoolExhausted with that target. Record the first attempt’s
ContextWindowExceeded via record_error before handling the fallback, while
preserving existing weak-to-strong retry behavior. Extend run’s tests to cover
overflow when the initially selected target is already the fallback target.
| ``` | ||
|
|
||
| Equivalent to: `switchyard[server,cli]` | ||
| Equivalent to: `nemo-switchyard[server,cli,tracing,intake,affinity-redis]` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files and inspect the exact snippets.
git ls-files | rg '^(pyproject\.toml|INSTALLATION\.md|README\.md)$'
printf '\n--- pyproject.toml ---\n'
cat -n pyproject.toml | sed -n '1,260p'
printf '\n--- INSTALLATION.md (around line 70) ---\n'
cat -n INSTALLATION.md | sed -n '55,85p'
printf '\n--- README.md (around line 219) ---\n'
cat -n README.md | sed -n '205,230p'Repository: NVIDIA-NeMo/Switchyard
Length of output: 10838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for all extra definitions/usages to confirm canonical naming.
rg -n --hidden --glob '!**/.git/**' --glob 'pyproject.toml' --glob 'README.md' --glob 'INSTALLATION.md' \
'(affinity-redis|redis-affinity|\[all\]|\[server,cli,tracing,intake,affinity-redis\])' .Repository: NVIDIA-NeMo/Switchyard
Length of output: 729
Use affinity-redis consistently in the docs.
pyproject.toml defines the extra as affinity-redis, but the README describes it as redis-affinity. Update the README wording to match the manifest.
📍 Affects 2 files
INSTALLATION.md#L70-L70(this comment)README.md#L219-L219
🤖 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 `@INSTALLATION.md` at line 70, The documentation uses inconsistent extra
naming. In INSTALLATION.md:70-70 and README.md:219-219, update the documented
extra name to affinity-redis to match pyproject.toml, replacing any
redis-affinity wording.
Comment on lines
+425
to
+437
| env_api_key = resolve_provider_connectivity( | ||
| cli_api_key=None, | ||
| cli_base_url=None, | ||
| api_key_env_vars=("OPENROUTER_API_KEY", "NVIDIA_API_KEY", "OPENAI_API_KEY"), | ||
| base_url_env_vars=("OPENROUTER_BASE_URL", "NVIDIA_BASE_URL", "OPENAI_BASE_URL"), | ||
| secrets=load_secrets(), | ||
| secrets_section_priority=DEFAULT_SECRETS_SECTION_PRIORITY, | ||
| default_provider=provider, | ||
| ).api_key | ||
| prompt_default_api_key = ( | ||
| existing_api_key | ||
| or request.prompt_default_api_key | ||
| or env_api_key |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the resolved provider with its environment credential.
resolve_provider_connectivity() can select openai/nvidia from the API-key environment variable, but this code discards .provider. For example, an OPENAI_API_KEY can be saved under nvidia while retaining NVIDIA’s base URL. Carry the resolved provider through config/credential persistence, or only accept an environment key matching the selected provider.
🤖 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 `@switchyard/cli/configure_command.py` around lines 425 - 437, Update the
configure flow around resolve_provider_connectivity() to retain its resolved
provider alongside env_api_key, and use that provider when persisting the
selected configuration and credentials. Ensure an environment key cannot be
saved under a different provider while retaining the selected provider’s base
URL; alternatively restrict environment-key resolution to the selected provider.
| or getattr(args, "classifier_model", None) | ||
| ): | ||
| return | ||
| if _OPENROUTER_URL_MARKER in base_url: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Parse the endpoint hostname instead of using a substring check.
"openrouter.ai" in base_url accepts unrelated hosts such as https://notopenrouter.ai (and can reject uppercase hostnames), bypassing the fail-fast guard and launching an unreachable default trio.
Proposed fix
+from urllib.parse import urlsplit
+
- if _OPENROUTER_URL_MARKER in base_url:
+ hostname = urlsplit(base_url).hostname or ""
+ if (
+ hostname == _OPENROUTER_URL_MARKER
+ or hostname.endswith(f".{_OPENROUTER_URL_MARKER}")
+ ):
return📝 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.
Suggested change
| if _OPENROUTER_URL_MARKER in base_url: | |
| from urllib.parse import urlsplit | |
| hostname = urlsplit(base_url).hostname or "" | |
| if ( | |
| hostname == _OPENROUTER_URL_MARKER | |
| or hostname.endswith(f".{_OPENROUTER_URL_MARKER}") | |
| ): | |
| return |
🤖 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 `@switchyard/cli/launch_command.py` at line 127, Update the endpoint check in
the launch command around _OPENROUTER_URL_MARKER to parse base_url and compare
its normalized hostname against the intended OpenRouter host, rather than
checking for a substring. Ensure case-insensitive matching and reject unrelated
hosts such as notopenrouter.ai so the fail-fast guard remains effective.
Comment on lines
+1463
to
+1470
| # Only strip a '--' that precedes the subcommand token; a '--' after it is | ||
| # the harness separator (e.g. `launch claude ... -- --version`) and must | ||
| # survive so forwarded args reach the launcher instead of tripping argparse. | ||
| _SUBCOMMANDS = ("serve", "configure", "launch", "verify") | ||
| sep_idx = argv.index("--") if "--" in argv else len(argv) | ||
| cmd_idx = next((i for i, t in enumerate(argv) if t in _SUBCOMMANDS), len(argv)) | ||
| if sep_idx < cmd_idx: | ||
| argv.pop(sep_idx) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not infer the subcommand from arbitrary option values.
Line 1468 treats any token named serve, launch, etc. as the command. switchyard --routing-profiles launch -- serve therefore leaves the separator in place and fails parsing. Strip the visual separator only when the following token is a recognized subcommand, or parse the global-option prefix structurally.
🤖 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 `@switchyard/cli/switchyard_cli.py` around lines 1463 - 1470, Update the
subcommand detection around _SUBCOMMANDS, sep_idx, and cmd_idx so option values
are not mistaken for the command. Identify the subcommand only from the
structurally valid global-option prefix, or strip the separator only when its
following token is a recognized subcommand; preserve separators intended for
forwarding after the actual command.
Comment on lines
+509
to
+511
| # OpenRouter endpoint: the zero-flag default trio is OpenRouter-only, so | ||
| # the deterministic default only dispatches when the resolved endpoint is | ||
| # OpenRouter (a non-OpenRouter provider is rejected — see NvBug 6401771). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the tracker reference from this source comment.
NvBug 6401771 is a project-management reference rather than behavior documentation.
🤖 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 `@tests/test_launch_codex.py` around lines 509 - 511, Remove the “NvBug
6401771” tracker reference from the source comment near the OpenRouter endpoint
logic, while preserving the explanation that non-OpenRouter providers are
rejected.
Source: Coding guidelines
Comment on lines
+361
to
+368
| monkeypatch.setattr( | ||
| "switchyard.cli.configure_command.fetch_model_ids", | ||
| lambda base_url, api_key: captured.update(base_url=base_url, api_key=api_key) or [], | ||
| ) | ||
| monkeypatch.setattr( | ||
| "switchyard.cli.configure_command.render_models", | ||
| lambda model_ids, request: "", | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Relevant test files\n'
git ls-files 'tests/test_user_config.py' 'tests/test_launch_claude_deterministic.py' 'tests/test_launch_codex.py' 'conftest.py' 'tests/conftest.py'
printf '\n## File sizes\n'
wc -l tests/test_user_config.py tests/test_launch_claude_deterministic.py tests/test_launch_codex.py 2>/dev/null || true
printf '\n## Search for mocker usage and monkeypatch patterns\n'
rg -n '\bmocker\b|monkeypatch\.setattr|monkeypatch\.setenv|pytest-mock|pytest_plugins' tests/test_user_config.py tests/test_launch_claude_deterministic.py tests/test_launch_codex.py tests/conftest.py conftest.py 2>/dev/null || true
printf '\n## Read around the cited regions\n'
sed -n '330,420p' tests/test_user_config.py
printf '\n---\n'
sed -n '250,350p' tests/test_launch_claude_deterministic.py
printf '\n---\n'
sed -n '480,540p' tests/test_launch_codex.py
printf '\n## Search for any repo guidance on monkeypatch vs mocker in tests\n'
rg -n 'pytest-mock|mocker.patch|use .*mocker|monkeypatch' tests README.md pyproject.toml setup.cfg tox.ini 2>/dev/null || trueRepository: NVIDIA-NeMo/Switchyard
Length of output: 50378
Use mocker.patch for these function stubs.
tests/test_user_config.py#L361-L398,tests/test_launch_claude_deterministic.py#L271-L333, andtests/test_launch_codex.py#L512-L515: replace themonkeypatch.setattrcalls that stub functions withmocker.patch; keepmonkeypatchfor env vars.
📍 Affects 3 files
tests/test_user_config.py#L361-L368(this comment)tests/test_user_config.py#L390-L398tests/test_launch_claude_deterministic.py#L271-L275tests/test_launch_claude_deterministic.py#L292-L295tests/test_launch_claude_deterministic.py#L319-L333tests/test_launch_codex.py#L512-L515
🤖 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 `@tests/test_user_config.py` around lines 361 - 368, Replace function-stubbing
monkeypatch.setattr calls with mocker.patch while preserving each stub’s
behavior; retain monkeypatch only for environment variables. Apply this in
tests/test_user_config.py ranges 361-368 and 390-398,
tests/test_launch_claude_deterministic.py ranges 271-275, 292-295, and 319-333,
and tests/test_launch_codex.py range 512-515.
Source: Coding guidelines
Contributor
Author
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.
Clears the open QA-filed bugs against the OSS builds, grouped by area. Verified against current
main:cargo build,cargo clippy -D warnings, strict rustdoc (RUSTDOCFLAGS="-D warnings" cargo doc),cargo test,ruff,mypy --strict, and the full non-integrationpytestsuite (one unrelated pre-existing failure intest_verify.py::test_secrets_file_wins_when_no_cli_or_env, which also fails on cleanmainon this dev box).libsy (Rust)
[0,1]) now fail open to strong instead of being treated as a below-threshold verdict and silently downgraded to weak.cargo package -p switchyard-libsysucceeds: theswitchyard-protocolpath dependency now carries a version.v2 server / routing (Rust)
GET /v1/modelsreports realtool_callingandcontext_windowcapability metadata instead ofnull.context_length_exceeded) instead of a 500server_error, on all inbound APIs.fallback_target_on_evictand retries once against the fallback target on a context-window overflow, matchingstage_router/llm_routing.CLI
launch <agent> -- <args>forwards documented harness args after a single--(the separator is only stripped before the subcommand, so the doubled--workaround is no longer needed).launchagainst a non-OpenRouter provider fails fast with guidance instead of launching the OpenRouter-only default trio against an incompatible endpoint.launch --dry-runreports a routing-profiles bundle asroute: bundle(with the bundle path and route ids) instead ofroute: single.configure --list-modelsandconfigurehonor the saveddefault_provider(the--providerflag no longer shadows it), and non-interactiveconfigureresolves an API key from the environment/secrets instead of requiring--api-key.serve --configwith a duplicate model id prints a one-line diagnostic instead of a raw traceback.servewith no config points at--configfirst (the v2 path), with--routing-profilesas the legacy option.HTTP_PROXYno longer breaks the127.0.0.1checks.Stats
/v1/routing/stats(request count and tokens under thenoopmodel) instead of being invisible.Docs / examples
[all]extra description, and the in-repo exampleroute.yaml.${VAR}references literal, so the referenced env vars must be set atserve/launchtime.SwitchyardRecipesAPI.Already correct on
main(no change here)The HTTP error-envelope consolidation, fail_open classifier observability, and v2 Codex Responses token stats were already fixed on
main; they only need QA verification.Summary by CodeRabbit
New Features
Bug Fixes
Documentation