Fix Bug 14: Timeout should fall back rather than emit a same-model retry message - #314
seonghobae wants to merge 3 commits into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
More reviews will be available in 7 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTimeout errors are reclassified to exclude same-model retry eligibility and flow directly to fallback-model evaluation instead. The function signature and test cases are updated to enforce this policy: the retry classifier no longer takes a model parameter, and test scenarios now expect fallback behavior rather than same-model retries for timeouts. ChangesTimeout-retry policy update and test alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR governance metadata gate is not ready for
|
|
@codex[agent] 스스로 평가하고, 스스로 판단하고, 스스로 PR 후 리뷰를 검토하여 스스로 그 다음 행동을 수행하세요. |
I have successfully verified the bug locally and modified |
Co-authored-by: seonghobae <8172694+seonghobae@users.noreply.github.com>
Self-review done: the timeout change is correct but it broke the Strix gate self-test expectations (Gemini timeout cases). Updated |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
This review was skipped because it would exceed your organization's monthly flex usage limit. Raise the limit in billing settings or wait until the next billing period resets limits.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/ci/strix_quick_gate.sh (1)
2042-2044: ⚡ Quick winCorrect this comment to reflect existing provider-specific fallback behavior.
This comment says there is no provider-specific fallback path, but the script does implement provider-specific fallback selection (e.g., Vertex/Gemini). Please align wording to prevent future misreads.
Suggested wording update
-## remaining budget retrying the same slow model. All models have no -## provider-specific fallback path in this gate, so LLM timeouts trigger -## a fallback model evaluation directly. +## remaining budget retrying the same slow model. Timeouts are excluded from +## same-model retry for every provider, so LLM timeouts trigger direct +## fallback-model evaluation using each provider's configured fallback list.🤖 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 `@scripts/ci/strix_quick_gate.sh` around lines 2042 - 2044, Update the misleading comment "remaining budget retrying the same slow model. All models have no provider-specific fallback path in this gate, so LLM timeouts trigger a fallback model evaluation directly." to accurately state that provider-specific fallback selection is implemented (for example Vertex/Gemini) and that on LLM timeouts the gate will invoke provider-specific fallback logic rather than a generic-only fallback; replace "no provider-specific fallback path" with wording like "provider-specific fallback paths (e.g., Vertex/Gemini) are supported, so LLM timeouts trigger the provider-specific fallback model selection" to prevent future confusion.scripts/ci/test_strix_quick_gate.sh (1)
4515-4526: ⚡ Quick winConsider renaming test to reflect fallback behavior.
The test name
gemini-timeout-retry-same-model-successimplies that a same-model retry succeeds, but the test actually verifies that timeouts trigger immediate fallback tovertex_ai/fallback-oneinstead of retryinggemini/retry-timeout-primary. The expected model sequence"gemini/retry-timeout-primary|vertex_ai/fallback-one"and success message about fallback confirm this.Consider renaming to something like
gemini-timeout-immediate-fallback-despite-retry-configto avoid confusion for future maintainers.🤖 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 `@scripts/ci/test_strix_quick_gate.sh` around lines 4515 - 4526, The test name passed to run_gate_case ("gemini-timeout-retry-same-model-success") is misleading because the test verifies immediate fallback to vertex_ai/fallback-one instead of retrying; change the first argument to a clearer name such as "gemini-timeout-immediate-fallback-despite-retry-config" (or similar) to reflect fallback behavior while leaving the rest of the run_gate_case invocation (expected sequence "gemini/retry-timeout-primary|vertex_ai/fallback-one", success message, etc.) unchanged; update only the first string argument in the run_gate_case call so logs and test output match the actual 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.
Nitpick comments:
In `@scripts/ci/strix_quick_gate.sh`:
- Around line 2042-2044: Update the misleading comment "remaining budget
retrying the same slow model. All models have no provider-specific fallback path
in this gate, so LLM timeouts trigger a fallback model evaluation directly." to
accurately state that provider-specific fallback selection is implemented (for
example Vertex/Gemini) and that on LLM timeouts the gate will invoke
provider-specific fallback logic rather than a generic-only fallback; replace
"no provider-specific fallback path" with wording like "provider-specific
fallback paths (e.g., Vertex/Gemini) are supported, so LLM timeouts trigger the
provider-specific fallback model selection" to prevent future confusion.
In `@scripts/ci/test_strix_quick_gate.sh`:
- Around line 4515-4526: The test name passed to run_gate_case
("gemini-timeout-retry-same-model-success") is misleading because the test
verifies immediate fallback to vertex_ai/fallback-one instead of retrying;
change the first argument to a clearer name such as
"gemini-timeout-immediate-fallback-despite-retry-config" (or similar) to reflect
fallback behavior while leaving the rest of the run_gate_case invocation
(expected sequence "gemini/retry-timeout-primary|vertex_ai/fallback-one",
success message, etc.) unchanged; update only the first string argument in the
run_gate_case call so logs and test output match the actual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1faf749d-dadf-40df-bc18-4547bde61079
📒 Files selected for processing (2)
scripts/ci/strix_quick_gate.shscripts/ci/test_strix_quick_gate.sh
Render
Destroyed
|
Superseded by the current master after #315 and the existing Bug 14 coverage. This branch is conflicting and its remaining diff weakens the hardened PR changed-file fallback from #315 while changing timeout retry semantics that current coverage already verifies for the Vertex Strix path. Keeping it open would preserve a stale failing Strix PR rather than a mergeable fix. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
This branch was successfully deployed
2 participants
Fixes Bug 14 where a timeout would emit a same-model retry message for non-Vertex models before falling back, instead of falling back directly.
is_transient_same_model_retry_errorto unconditionally return1on timeout.PR created automatically by Jules for task 10022452761947662593 started by @seonghobae
Summary by CodeRabbit