Repository navigation
fix(gateway): dedup provider-error status and final response for adapters without send_or_update_status #72144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -534,6 +534,32 @@ def _looks_like_gateway_provider_error(text: str) -> bool: | |
| return bool(_GATEWAY_PROVIDER_ERROR_SHAPE_RE.search(body)) | ||
|
|
||
|
|
||
| # Issue #72131: track provider-error status text sent via the plain-send fallback | ||
| # so that _sanitize_gateway_final_response can dedup identical final responses. | ||
| _last_sent_provider_error_status: Optional[str] = None | ||
| # Set to True when _sanitize_gateway_final_response suppresses output due to dedup. | ||
| _deduped_provider_error: bool = False | ||
|
|
||
|
|
||
| def _record_provider_error_status(text: str) -> None: | ||
| """Record a provider-error status that was sent as a persistent message. | ||
|
|
||
| Called when _send_or_update_status_coro falls back to plain send() (adapter | ||
| lacks send_or_update_status) and the content is a provider-error reply. | ||
| """ | ||
| global _last_sent_provider_error_status, _deduped_provider_error | ||
| _last_sent_provider_error_status = text | ||
| _deduped_provider_error = False # reset; only meaningful after sanitize | ||
|
|
||
|
|
||
| def _should_dedup_final_response(text: str) -> bool: | ||
| """Return True if text duplicates a provider-error status already sent.""" | ||
| return bool( | ||
| _last_sent_provider_error_status | ||
| and text == _last_sent_provider_error_status | ||
| ) | ||
|
|
||
|
|
||
| def _sanitize_gateway_final_response(platform: Any, text: str) -> str: | ||
| """Sanitize final gateway replies before sending them to chat surfaces. | ||
|
|
||
|
|
@@ -556,7 +582,17 @@ def _sanitize_gateway_final_response(platform: Any, text: str) -> str: | |
|
|
||
| redacted = _redact_gateway_user_facing_secrets(str(text)) | ||
| if _looks_like_gateway_provider_error(redacted): | ||
| return _gateway_provider_error_reply(redacted) | ||
| result = _gateway_provider_error_reply(redacted) | ||
| # Issue #72131: if this identical provider-error text was already | ||
| # delivered as a mid-run status (via the plain-send fallback), | ||
| # suppress the duplicate final response. | ||
| global _deduped_provider_error | ||
| if _should_dedup_final_response(result): | ||
| _deduped_provider_error = True | ||
| return "" | ||
| _deduped_provider_error = False | ||
| return result | ||
| _deduped_provider_error = False | ||
| return redacted | ||
|
|
||
|
|
||
|
|
@@ -611,10 +647,18 @@ async def _send_or_update_status_coro(adapter, chat_id, status_key, content, met | |
| Issue #30045: adapters that implement send_or_update_status (currently | ||
| Telegram) edit the previous bubble for the same status_key instead of | ||
| appending a new one. Adapters without the method fall back to plain send. | ||
|
|
||
| Issue #72131: when the fallback plain-send path is used and the content | ||
| is a provider-error reply, record it so the final-response dedup can | ||
| suppress the identical persistent message that would otherwise follow. | ||
| """ | ||
| sender = getattr(adapter, "send_or_update_status", None) | ||
| if callable(sender): | ||
| return await sender(chat_id, status_key, content, metadata=metadata) | ||
| # Fallback: plain send() → status becomes a persistent message. | ||
| # If it's a provider-error reply, record it for final-response dedup. | ||
| if _looks_like_gateway_provider_error(content): | ||
| _record_provider_error_status(content) | ||
| return await adapter.send(chat_id, content, metadata=metadata) | ||
|
|
||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This tests the already-sanitized status string against an envelope matcher. The rate-limit status produced by |
||
|
|
@@ -22391,7 +22435,10 @@ def _approval_notify_sync(approval_data: dict) -> None: | |
| result, final_response or "", history_len=len(agent_history), | ||
| ) | ||
| final_response = _sanitize_gateway_final_response(source.platform, final_response) | ||
| if not final_response: | ||
| # Issue #72131: if dedup suppressed a provider-error final | ||
| # response (already delivered as status), do NOT fall back to | ||
| # the raw error text — the user already has the sanitized reply. | ||
| if not final_response and not _deduped_provider_error: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only prevents the inner fallback. The outer gateway path later calls |
||
| final_response = f"⚠️ {result['error']}" if result.get("error") else "" | ||
| return { | ||
| "final_response": final_response, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| """Tests for provider-error dedup between status and final response (issue #72131).""" | ||
|
|
||
| import pytest | ||
|
|
||
| from gateway.run import ( | ||
| _record_provider_error_status, | ||
| _sanitize_gateway_final_response, | ||
| _should_dedup_final_response, | ||
| _last_sent_provider_error_status, | ||
| _deduped_provider_error, | ||
| ) | ||
|
|
||
|
|
||
| class TestProviderErrorDedup: | ||
| """Verify that _sanitize_gateway_final_response suppresses duplicates | ||
| when the same provider-error text was already sent as a status message | ||
| via the plain-send fallback (adapter without send_or_update_status). | ||
| """ | ||
|
|
||
| def setup_method(self): | ||
| """Reset module-level dedup state before each test.""" | ||
| import gateway.run as _run | ||
| _run._last_sent_provider_error_status = None | ||
| _run._deduped_provider_error = False | ||
|
|
||
| # -- recording -- | ||
|
|
||
| def test_record_sets_tracking(self): | ||
| _record_provider_error_status("⏱️ rate limited") | ||
| assert _should_dedup_final_response("⏱️ rate limited") is True | ||
| assert _should_dedup_final_response("something else") is False | ||
|
|
||
| def test_record_resets_dedup_flag(self): | ||
| import gateway.run as _run | ||
| _run._deduped_provider_error = True | ||
| _record_provider_error_status("⏱️ rate limited") | ||
| assert _run._deduped_provider_error is False | ||
|
|
||
| # -- dedup logic -- | ||
|
|
||
| def test_no_prior_status_no_dedup(self): | ||
| """Without a prior status, no dedup should fire.""" | ||
| assert _should_dedup_final_response("anything") is False | ||
|
|
||
| def test_identical_text_dedups(self): | ||
| _record_provider_error_status( | ||
| "⏱️ The model provider is rate-limiting requests. " | ||
| "Please wait a moment and try again." | ||
| ) | ||
| assert _should_dedup_final_response( | ||
| "⏱️ The model provider is rate-limiting requests. " | ||
| "Please wait a moment and try again." | ||
| ) is True | ||
|
|
||
| def test_different_text_no_dedup(self): | ||
| _record_provider_error_status("⏱️ rate limited") | ||
| assert _should_dedup_final_response("⚠️ auth failed") is False | ||
|
|
||
| # -- sanitize dedup -- | ||
|
|
||
| @pytest.mark.parametrize("platform", ["whatsapp", "discord", "slack", "signal", "matrix"]) | ||
| def test_sanitize_dedups_identical_provider_error(self, platform): | ||
| """When the same provider-error text was sent as a status, the final | ||
| response should be empty (deduped).""" | ||
| raw = "API call failed after 3 retries: HTTP 429 rate limited" | ||
|
|
||
| # Simulate: status was sent via plain-send fallback | ||
| _record_provider_error_status( | ||
| "⏱️ The model provider is rate-limiting requests. " | ||
| "Please wait a moment and try again." | ||
| ) | ||
|
|
||
| # Now the final response comes in — should be deduped | ||
| result = _sanitize_gateway_final_response(platform, raw) | ||
| assert result == "" | ||
|
|
||
| import gateway.run as _run | ||
| assert _run._deduped_provider_error is True | ||
|
|
||
| @pytest.mark.parametrize("platform", ["whatsapp", "discord", "slack", "signal", "matrix"]) | ||
| def test_sanitize_passes_when_no_prior_status(self, platform): | ||
| """Without a prior status, provider errors should be sanitized normally.""" | ||
| raw = "API call failed after 3 retries: HTTP 429 rate limited" | ||
|
|
||
| result = _sanitize_gateway_final_response(platform, raw) | ||
| assert result == ( | ||
| "⏱️ The model provider is rate-limiting requests. " | ||
| "Please wait a moment and try again." | ||
| ) | ||
|
|
||
| @pytest.mark.parametrize("platform", ["whatsapp", "discord", "slack", "signal", "matrix"]) | ||
| def test_sanitize_passes_when_text_differs(self, platform): | ||
| """If the final response text differs from the status, pass it through.""" | ||
| raw = "API call failed after 3 retries: HTTP 401 unauthorized" | ||
|
|
||
| # A different error was sent as status | ||
| _record_provider_error_status( | ||
| "⏱️ The model provider is rate-limiting requests. " | ||
| "Please wait a moment and try again." | ||
| ) | ||
|
|
||
| result = _sanitize_gateway_final_response(platform, raw) | ||
| # Auth error produces different text | ||
| assert "authentication failed" in result.lower() or "provider" in result.lower() | ||
|
|
||
| def test_local_platform_not_deduped(self): | ||
| """Local/CLI platforms pass raw text unchanged — no dedup needed.""" | ||
| raw = "API call failed after 3 retries: HTTP 429 rate limited" | ||
|
|
||
| _record_provider_error_status( | ||
| "⏱️ The model provider is rate-limiting requests. " | ||
| "Please wait a moment and try again." | ||
| ) | ||
|
|
||
| result = _sanitize_gateway_final_response("local", raw) | ||
| # Local platforms keep raw text | ||
| assert "429" in result | ||
| assert "rate" in result.lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_last_sent_provider_error_statusis process-global and is never scoped, consumed, or cleared at a run boundary. Two concurrent chats, or a later turn with the same friendly error text, can suppress an unrelated final reply. Keep this state on the affected turn/chat instead.