test(e2e): add model router routed guard for #3255 - #3601
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new E2E regression test (issue ChangesModel Router provider-routed inference E2E regression test
Sequence Diagram(s)sequenceDiagram
participant User as workflow_dispatch
participant Selector as select_regression_jobs
participant CIJob as model-router-provider-routed-inference-e2e
participant Sandbox as Sandbox
participant ModelRouter as Model Router
User->>Selector: submit jobs input
Selector->>CIJob: enable job when selected
CIJob->>Sandbox: run test script (onboard sandbox, call endpoints)
Sandbox->>ModelRouter: poll /health & /v1/chat/completions
ModelRouter-->>Sandbox: health status, routed completion responses
CIJob->>CIJob: upload logs on failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
E2E Advisor RecommendationRequired E2E: None Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/test-model-router-provider-routed-inference.sh`:
- Around line 19-36: is_routed_pong_response fails because the embedded Python
reads sys.stdin.read() while the here-doc (python3 - <<'PY') consumes stdin, so
the JSON payload passed via here-string is empty; fix by changing the function
to accept the payload as an argument and invoke Python so it reads from sys.argv
(not stdin): replace the here-doc usage in is_routed_pong_response with a Python
invocation that reads the payload from sys.argv[1] (or similar), and update
callers to call is_routed_pong_response "$response" instead of piping via stdin.
🪄 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: c5abdae9-fdc4-4efe-9fe8-55fc82c1c40a
📒 Files selected for processing (4)
.github/workflows/regression-e2e.yamltest/e2e/docs/parity-inventory.generated.jsontest/e2e/docs/parity-map.yamltest/e2e/test-model-router-provider-routed-inference.sh
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/test-model-router-provider-routed-inference.sh (1)
19-36:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPass the response payload via argv instead of stdin.
python3 - <<'PY'consumes stdin for the here-doc itself, sosys.stdin.read()is empty when this function is called with<<<"$response". That makes the routed-response check fail even for valid JSON and turns this regression guard into a false negative.🔧 Minimal fix
is_routed_pong_response() { - python3 - <<'PY' + local raw="$1" + python3 - "$raw" <<'PY' import json, re, sys -raw = sys.stdin.read() +raw = sys.argv[1] try: data = json.loads(raw) except Exception: raise SystemExit(1) @@ - if is_routed_pong_response <<<"$response"; then + if is_routed_pong_response "$response"; then @@ -if is_routed_pong_response <<<"$response"; then +if is_routed_pong_response "$response"; thenYou can verify the shell/Python stdin behavior with:
#!/bin/bash set -euo pipefail sed -n '19,36p;161p;171p' test/e2e/test-model-router-provider-routed-inference.sh bash -c ' f() { python3 - <<'"'"'PY'"'"' import sys print(len(sys.stdin.read())) PY } f <<<"abc" 'Expected result: the demo prints
0, confirming the here-doc, not the here-string payload, reaches Python stdin.Also applies to: 161-161, 171-171
🤖 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 `@test/e2e/test-model-router-provider-routed-inference.sh` around lines 19 - 36, The is_routed_pong_response bash helper consumes the response via a here-doc which prevents the caller's here-string (<<<"$response") from reaching Python's stdin, causing false negatives; update the Python invocation inside is_routed_pong_response to accept the JSON payload as an argv parameter (e.g., read from sys.argv[1]) instead of sys.stdin.read(), and adjust any other similar helpers called at lines referenced (around 161 and 171) so they also pass the response via argv; locate the Python block inside is_routed_pong_response and change the input code path from sys.stdin.read() to using sys.argv[1] (or equivalent) and ensure the bash call forwards "$response" as an argument.
🤖 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.
Duplicate comments:
In `@test/e2e/test-model-router-provider-routed-inference.sh`:
- Around line 19-36: The is_routed_pong_response bash helper consumes the
response via a here-doc which prevents the caller's here-string (<<<"$response")
from reaching Python's stdin, causing false negatives; update the Python
invocation inside is_routed_pong_response to accept the JSON payload as an argv
parameter (e.g., read from sys.argv[1]) instead of sys.stdin.read(), and adjust
any other similar helpers called at lines referenced (around 161 and 171) so
they also pass the response via argv; locate the Python block inside
is_routed_pong_response and change the input code path from sys.stdin.read() to
using sys.argv[1] (or equivalent) and ensure the bash call forwards "$response"
as an argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dd37f849-8064-4b16-a1f8-9097b4b42104
📒 Files selected for processing (2)
test/e2e/docs/parity-inventory.generated.jsontest/e2e/test-model-router-provider-routed-inference.sh
✅ Files skipped from review due to trivial changes (1)
- test/e2e/docs/parity-inventory.generated.json
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/test-model-router-provider-routed-inference.sh (1)
30-47:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix routed-response parsing: stdin is consumed by the heredoc, so valid responses are never detected.
At Line 31,
python3 - <<'PY'feeds the script through stdin; then Line 33 reads stdin for JSON, which is empty. With current call sites on Line 172 and Line 182,is_routed_pong_responsewill consistently fail parsing.Suggested minimal fix
is_routed_pong_response() { - python3 - <<'PY' + local raw="$1" + python3 - "$raw" <<'PY' import json, re, sys -raw = sys.stdin.read() +raw = sys.argv[1] try: data = json.loads(raw) except Exception: raise SystemExit(1) @@ raise SystemExit(0 if ok_model and ok_content else 1) PY } @@ - if is_routed_pong_response <<<"$response"; then + if is_routed_pong_response "$response"; then @@ -if is_routed_pong_response <<<"$response"; then +if is_routed_pong_response "$response"; then#!/usr/bin/env bash set -euo pipefail # Verify the stdin/heredoc behavior causing the failure. broken() { python3 - <<'PY' import sys print(len(sys.stdin.read())) PY } echo "broken (expected 0): $(broken <<< 'abc')" fixed() { local raw="$1" python3 - "$raw" <<'PY' import sys print(len(sys.argv[1])) PY } echo "fixed (expected 3): $(fixed 'abc')"Also applies to: 172-172, 182-182
🤖 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 `@test/e2e/test-model-router-provider-routed-inference.sh` around lines 30 - 47, The is_routed_pong_response helper is reading JSON from stdin but the heredoc used to invoke python consumes stdin so the script always sees empty input; fix by passing the raw response into Python as an argument (or env var) instead of relying on sys.stdin: change the bash wrapper that defines is_routed_pong_response to capture the heredoc into a shell variable (e.g., raw) and invoke python with that value as an argument, then update the Python code inside is_routed_pong_response to parse sys.argv[1] (or the env var) for the JSON and keep the existing model/choices/content checks; also update the places that call is_routed_pong_response to pass the response string rather than relying on stdin.
🤖 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.
Duplicate comments:
In `@test/e2e/test-model-router-provider-routed-inference.sh`:
- Around line 30-47: The is_routed_pong_response helper is reading JSON from
stdin but the heredoc used to invoke python consumes stdin so the script always
sees empty input; fix by passing the raw response into Python as an argument (or
env var) instead of relying on sys.stdin: change the bash wrapper that defines
is_routed_pong_response to capture the heredoc into a shell variable (e.g., raw)
and invoke python with that value as an argument, then update the Python code
inside is_routed_pong_response to parse sys.argv[1] (or the env var) for the
JSON and keep the existing model/choices/content checks; also update the places
that call is_routed_pong_response to pass the response string rather than
relying on stdin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48ab1c12-a672-414c-81f0-9f0381cf49fe
📒 Files selected for processing (1)
test/e2e/test-model-router-provider-routed-inference.sh
cv
approved these changes
May 15, 2026
This was referenced May 18, 2026
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
4 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
Adds a regression E2E guard for model-router provider-routed inference so the routed inference path is covered in the regression workflow. This replaces closed PR #3594 from the rebased branch because repository rules blocked force-pushing the original PR branch.
Related Issue
Refs #3255
Changes
test/e2e/test-model-router-provider-routed-inference.shfor provider-routed inference coverage..github/workflows/regression-e2e.yamlto include the new regression guard.test/e2e/docs/parity-map.yamlandtest/e2e/docs/parity-inventory.generated.json.pr-3594-rebased-main.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
Tests
Chores
Documentation