fix(ai): retry without stop param on 400 unsupported_parameter#1544
fix(ai): retry without stop param on 400 unsupported_parameter#1544kiet08hogit wants to merge 2 commits into
Conversation
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
|
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:
📝 WalkthroughWalkthroughAdded shared invocation and streaming fallbacks that retry without ChangesStop parameter compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChatBase
participant StopFallback
participant LLMBackend
ChatBase->>StopFallback: invoke or stream with stop
StopFallback->>LLMBackend: attempt call with stop
LLMBackend-->>StopFallback: unsupported stop error
StopFallback->>LLMBackend: retry without stop
LLMBackend-->>ChatBase: response or streamed chunks
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 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 `@nodes/test/agent_crewai/test_stop_words.py`:
- Around line 399-401: In test_both_empty_yields_empty, replace the permissive
assertion with an exact result == [] check. Align the test name and coverage by
either renaming it to reflect a None and empty-list input or adding a separate
assertion/test for _resolve_stop([], []).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e53ad5c8-1f31-4a0d-be26-19924e6ead6b
📒 Files selected for processing (2)
nodes/src/nodes/agent_crewai/crewai_base.pynodes/test/agent_crewai/test_stop_words.py
| def test_both_empty_yields_empty(self): | ||
| result = _resolve_stop(None, []) | ||
| assert result == [] or result is None # either is acceptable (no stop) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten the assertion in test_both_empty_yields_empty.
_resolve_stop(None, []) always returns [] (since None or [] evaluates to []), so the or result is None branch is dead code. The assertion should be exact. Additionally, the test name says "both empty" but passes None as the first argument — consider also testing _resolve_stop([], []) or renaming.
🧪 Proposed fix
def test_both_empty_yields_empty(self):
- result = _resolve_stop(None, [])
- assert result == [] or result is None # either is acceptable (no stop)
+ # None (key absent) and [] (explicit empty) both resolve to no stop.
+ assert _resolve_stop(None, []) == []
+ assert _resolve_stop([], []) == []📝 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.
| def test_both_empty_yields_empty(self): | |
| result = _resolve_stop(None, []) | |
| assert result == [] or result is None # either is acceptable (no stop) | |
| def test_both_empty_yields_empty(self): | |
| # None (key absent) and [] (explicit empty) both resolve to no stop. | |
| assert _resolve_stop(None, []) == [] | |
| assert _resolve_stop([], []) == [] |
🤖 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 `@nodes/test/agent_crewai/test_stop_words.py` around lines 399 - 401, In
test_both_empty_yields_empty, replace the permissive assertion with an exact
result == [] check. Align the test name and coverage by either renaming it to
reflect a None and empty-list input or adding a separate assertion/test for
_resolve_stop([], []).
Fixes rocketride-org#1544. Prevents pipeline crashes on OpenAI reasoning models (gpt-5.x, o-series) by catching the 400 Bad Request error when they reject the stop parameter. The chat loop now intercepts the error and gracefully retries the request without stop sequences.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/ai/src/ai/common/chat.py`:
- Around line 53-68: Move the llm.stream(prompt, **stop_kwargs) call inside the
try block of _stream_with_stop_fallback, keeping iterator consumption and
fallback handling within the same exception scope so synchronous stream creation
errors trigger the existing stop-parameter retry logic.
In `@packages/ai/tests/ai/common/test_chat_fallback.py`:
- Around line 36-80: Add negative tests covering _invoke_with_stop_fallback when
stop_kwargs is empty and when the raised error does not match the stop-rejection
pattern; assert the ValueError propagates and the mock records exactly one
invocation, using an always-failing mock such as MockAlwaysFailInvokeLLM.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c265bf4b-3a52-4d31-ac4d-4646fbd069aa
📒 Files selected for processing (2)
packages/ai/src/ai/common/chat.pypackages/ai/tests/ai/common/test_chat_fallback.py
bb58b01 to
83566e5
Compare
83566e5 to
358c9d1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ai/tests/ai/common/test_chat_fallback.py (1)
36-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNegative test cases for no-stop and non-matching-error paths still missing.
The current tests cover happy paths and stop-rejection recovery well, but there are no tests verifying the fallback does NOT retry when
stop_kwargsis empty or when the error doesn't match the stop-rejection pattern. These guard against false-positive retries. This was previously flagged and remains unaddressed.Suggested additional tests
class MockAlwaysFailInvokeLLM: """Mock that always raises, regardless of stop kwargs.""" def __init__(self, error_msg="unrelated error"): self.invoked_kwargs = [] self.error_msg = error_msg def invoke(self, prompt, **kwargs): self.invoked_kwargs.append(kwargs) raise ValueError(self.error_msg) def test_invoke_no_stop_does_not_retry(): """Without stop kwargs, errors propagate without retry.""" llm = MockAlwaysFailInvokeLLM() with pytest.raises(ValueError): _invoke_with_stop_fallback(llm, "Hello", {}) assert len(llm.invoked_kwargs) == 1 def test_invoke_non_matching_error_does_not_retry(): """Non-stop-related errors propagate without retry.""" llm = MockAlwaysFailInvokeLLM("authentication failed") with pytest.raises(ValueError, match="authentication failed"): _invoke_with_stop_fallback(llm, "Hello", {'stop': ['x']}) assert len(llm.invoked_kwargs) == 1🤖 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 `@packages/ai/tests/ai/common/test_chat_fallback.py` around lines 36 - 80, Add negative coverage for _invoke_with_stop_fallback: introduce an always-failing mock, then verify empty stop_kwargs propagates the error after one invocation and an unrelated error with stop kwargs also propagates without retry. Assert the expected ValueError messages and invocation count.
🤖 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 `@packages/ai/tests/ai/common/test_chat_fallback.py`:
- Around line 36-80: Add negative coverage for _invoke_with_stop_fallback:
introduce an always-failing mock, then verify empty stop_kwargs propagates the
error after one invocation and an unrelated error with stop kwargs also
propagates without retry. Assert the expected ValueError messages and invocation
count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6954250-209b-4f97-a46c-1649e3c57fd1
📒 Files selected for processing (1)
packages/ai/tests/ai/common/test_chat_fallback.py
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 `@packages/ai/src/ai/common/chat.py`:
- Around line 53-68: The _stream_with_stop_fallback function must only apply the
stop-parameter retry before emitting any output. Restrict the try/except to
stream creation and the initial next(it) call, yield the first chunk afterward,
and iterate over the existing stream outside the exception handler; retry
without stop only when creation or the first read raises a matching
unsupported/invalid-stop error.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: effb62f1-3ea3-4d5a-812d-418b9b7e956f
📒 Files selected for processing (2)
packages/ai/src/ai/common/chat.pypackages/ai/tests/ai/common/test_chat_fallback.py
Fixes rocketride-org#1538. Prevents pipeline crashes on OpenAI reasoning models (gpt-5.x, o-series) by catching the 400 Bad Request error when they reject the stop parameter. The chat loop now intercepts the error and gracefully retries the request without stop sequences.
358c9d1 to
3977d78
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ai/src/ai/common/chat.py (1)
53-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestrict retry to before first emission to prevent duplicate chunks.
yield firstandyield from itremain inside thetryblock. If the stream raises a stop-related error after the first chunk has already been emitted to the caller, theexcepthandler will retry withyield from llm.stream(prompt), replaying all chunks from the beginning and producing duplicates. Move the yields outside thetryso the fallback only applies to stream creation and the initialnext(it).Proposed fix
def _stream_with_stop_fallback(llm, prompt, stop_kwargs): """Stream from the LLM, but fallback to no stop kwargs if the API rejects them.""" try: it = llm.stream(prompt, **stop_kwargs) first = next(it) - yield first - yield from it except StopIteration: return except Exception as e: err_str = str(e).lower() if stop_kwargs and 'stop' in err_str and ('unsupported' in err_str or 'invalid' in err_str): warning(f"LLM rejected 'stop' parameter: {e}. Retrying stream without stop.") yield from llm.stream(prompt) + return else: raise + yield first + yield from it🤖 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 `@packages/ai/src/ai/common/chat.py` around lines 53 - 68, Update _stream_with_stop_fallback so the try block only creates the stream and retrieves its first item; move yield first and yield from it outside the try/except. This ensures stop-related errors after the first emission propagate instead of triggering a fallback that duplicates already-emitted chunks.
🤖 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 `@packages/ai/tests/ai/common/test_chat_fallback.py`:
- Around line 82-107: Add streaming fallback negative tests alongside
test_invoke_with_stop_fallback_empty_stop_kwargs and
test_invoke_with_stop_fallback_unrelated_error. Use MockStreamLLM and a failing
stream generator to verify _stream_with_stop_fallback re-raises
unsupported-parameter errors with empty stop_kwargs and unrelated errors with
stop_kwargs, while asserting only one stream invocation occurs in each case.
---
Duplicate comments:
In `@packages/ai/src/ai/common/chat.py`:
- Around line 53-68: Update _stream_with_stop_fallback so the try block only
creates the stream and retrieves its first item; move yield first and yield from
it outside the try/except. This ensures stop-related errors after the first
emission propagate instead of triggering a fallback that duplicates
already-emitted chunks.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 402ccff6-c0f2-4506-a0f0-6c78286b933b
📒 Files selected for processing (2)
packages/ai/src/ai/common/chat.pypackages/ai/tests/ai/common/test_chat_fallback.py
joshuadarron
left a comment
joshuadarron
left a comment
There was a problem hiding this comment.
The approach is right — this is exactly option 1 from #1538 (catch the 400, retry once without stop, rely on the post-hoc truncate_at_stop_words backstop, which I verified still applies on the agent path at ai/common/agent/agent.py:317). Test coverage of the invoke path is good, including the two negative cases. But three things need fixing before merge:
Blocking:
- Title/description don't match the diff. The title says
fix(agent_crewai): accept stop param from kwargsand the body describes a kwargs fallback inHostInvokeLLMplusTestKwargsStopFallbacktests — none of that is in this PR (the single commit touches onlyai/common/chat.py+test_chat_fallback.py). NoteHostInvokeLLMon develop already readsself.stop_sequences(nodes/src/nodes/agent_crewai/crewai_base.py:266), so if the kwargs commit was dropped intentionally, retitle tofix(ai): retry without stop param on 400 unsupported_parameterand rewrite the summary; if it wasn't intentional, the commit is missing. - Ruff fails: 18× Q000 (double quotes; repo enforces single quotes) — I ran ruff against the PR files to confirm.
python -m ruff check --fixclears all of them. - Mid-stream fallback can duplicate output — see inline comment on
_stream_with_stop_fallback.
Non-blocking suggestions:
- Consider caching a "this backend rejects stop" flag after the first fallback (e.g. on the wrapper) — as written, every agent step against a gpt-5.x/o-series model pays a failed round-trip first, and CrewAI's internal retries multiply that.
- Worth a code comment noting that on the retry path a hallucinated
\nObservation:can stream to the UI before the agent-level truncation trims it — inherent to the retry-without-stop approach, but future readers should know it's a known trade-off. - A test for an error raised after the first streamed chunk would lock in the no-duplicate behavior once the inline issue is fixed.
| first = next(it) | ||
| yield first | ||
| yield from it | ||
| except StopIteration: |
There was a problem hiding this comment.
🟡 risk: the except wraps the whole body including yield from it, so an error that matches the string filter after chunks were already yielded restarts the stream from scratch and duplicates everything already sent to the UI. The first = next(it) special-casing suggests the intent was to guard only stream creation + first chunk — restructure so only that part is inside the try:
try:
it = llm.stream(prompt, **stop_kwargs)
first = next(it)
except StopIteration:
return
except Exception as e:
err_str = str(e).lower()
if stop_kwargs and 'stop' in err_str and ('unsupported' in err_str or 'invalid' in err_str):
warning(f'LLM rejected stop parameter: {e}. Retrying stream without stop.')
yield from llm.stream(prompt)
return
raise
yield first
yield from itSame principle as the existing guard at _chat_string_responses ("Only retry non-streaming if nothing has reached the UI").
| return llm.invoke(prompt, **stop_kwargs) | ||
| except Exception as e: | ||
| err_str = str(e).lower() | ||
| if stop_kwargs and 'stop' in err_str and ('unsupported' in err_str or 'invalid' in err_str): |
There was a problem hiding this comment.
🟡 risk: matching any Exception whose text contains stop + (unsupported|invalid) is broad — e.g. a legitimately malformed stop-sequence config error (invalid_request_error ... stop_sequences) gets silently swallowed and retried without stop instead of surfacing. The OpenAI error is structured; prefer checking getattr(e, 'status_code', None) == 400 and/or code == 'unsupported_parameter' / param == 'stop' from e.body/e.code when present, keeping the string match only as a last-resort fallback for providers without structured errors. Consider extracting the predicate into one _is_stop_rejection(e) helper — it's duplicated verbatim in both functions.
kiet08hogit
changed the title
|
Thanks @joshuadarron for the detailed review! I've pushed an update that addresses all of your feedback. The error matching has been extracted into a more precise _is_stop_rejection helper that checks structured API errors first. I also fixed the mid-stream duplication bug by moving the yield loop outside the try/except block, and added the requested negative tests to ensure the fallback doesn't trigger on unrelated errors or after a chunk has already been emitted. Finally, I added a note about the UI hallucination trade-off, ran ruff to fix the quote formatting, and updated the PR title and description to match the new scope. |
2 participants
Fixes #1538. Prevents default models from crashing when they do not support the ReAct stop sequence during tool invocation.
Summary
stopsequence fromkwargsinHostInvokeLLM, fixing flows where CrewAI passes it directly as an argument rather than via contextvar.stopparameters in LLMinvokeandstreampaths. Retries the API request gracefully without the stop sequence.TestKwargsStopFallback) for the CrewAI wrapper, and regression tests (test_chat_fallback.py) for the OpenAI fallback logic.Type
fix
Testing
./builder testpassesChecklist
Linked Issue
Fixes #1538
Summary by CodeRabbit
Bug Fixes
Tests