From 0c6b94e49954f5f121bc20fa8b1014ba021096c9 Mon Sep 17 00:00:00 2001 From: Matt Earls <244358104+mearls0501@users.noreply.github.com> Date: Wed, 9 Sep 2026 06:59:10 -0400 Subject: [PATCH] fix(image-gen): preserve managed FAL billing errors Avoid retrying idempotent managed FAL submissions because the retry can mask the initial billing failure. Surface structured Nous billing diagnostics consistently for image and video paths, with hermetic regression coverage. (cherry picked from commit 289ce039e9a522dc8016ae4a512214c05d0a8bc0) --- plugins/video_gen/fal/__init__.py | 12 +++- tests/plugins/video_gen/test_fal_plugin.py | 55 ++++++++++++++++- tests/tools/test_fal_common.py | 33 ++++++++++ tests/tools/test_image_generation.py | 61 ++++++++++++++++++- tests/tools/test_managed_media_gateways.py | 14 ++++- ...st_video_generation_tool_surface_matrix.py | 8 +-- tools/fal_common.py | 49 ++++++++++++++- tools/image_generation_tool.py | 15 ++++- 8 files changed, 229 insertions(+), 18 deletions(-) diff --git a/plugins/video_gen/fal/__init__.py b/plugins/video_gen/fal/__init__.py index 9760891966..94841727e2 100644 --- a/plugins/video_gen/fal/__init__.py +++ b/plugins/video_gen/fal/__init__.py @@ -221,9 +221,19 @@ def _submit_fal_video_request(endpoint: str, arguments: Dict[str, Any]): try: return _get_managed_fal_video_client(managed_gateway).submit(endpoint, arguments=arguments, headers=headers) except Exception as exc: - from tools.fal_common import _extract_http_status + from tools.fal_common import _extract_http_status, _managed_fal_billing_error status = _extract_http_status(exc) if status is not None and 400 <= status < 500: + billing = _managed_fal_billing_error(exc) + if billing is not None: + raise ValueError( + f"Nous Subscription gateway rejected endpoint '{endpoint}' (HTTP {status}): " + f"{billing['message']} ({billing['error_code']}; " + f"{billing['code']}: {billing['detail']}). " + "This is a Nous Portal billing configuration issue, not a missing local API key. " + "The managed route cannot run this endpoint until Nous enables its billing meter; " + "a direct FAL_KEY is an optional bypass." + ) from exc raise ValueError(f"Nous Subscription gateway rejected endpoint '{endpoint}' (HTTP {status}). This model may not yet be enabled " f"on the Nous Portal's FAL proxy. Either:\n • Set FAL_KEY in your environment to use FAL.ai directly, or\n" f" • Pick a different model via `hermes tools` → Video Generation.") from exc diff --git a/tests/plugins/video_gen/test_fal_plugin.py b/tests/plugins/video_gen/test_fal_plugin.py index 821d2db24d..eec18e0318 100644 --- a/tests/plugins/video_gen/test_fal_plugin.py +++ b/tests/plugins/video_gen/test_fal_plugin.py @@ -29,6 +29,54 @@ def test_fal_provider_registers(): assert DEFAULT_MODEL in {"pixverse-v6", "ltx-2.3"} +def test_managed_billing_error_preserves_structured_diagnostic(monkeypatch): + """Video and image callers share the same Nous billing diagnosis.""" + from plugins.video_gen import fal as fal_plugin + + class Response: + status_code = 409 + + @staticmethod + def json(): + return { + "error": { + "code": "BILLING_ERROR", + "message": "Charge authorization failed", + "details": { + "upstreamPayload": { + "code": "unsupported_pricing_meter", + "error": "Unsupported resolver usage meter", + } + }, + } + } + + class BillingError(RuntimeError): + def __init__(self): + super().__init__("gateway billing failure") + self.response = Response() + + error = BillingError() + client = Mock() + client.submit.side_effect = error + monkeypatch.setattr(fal_plugin, "_load_fal_client", lambda: object()) + monkeypatch.setattr( + fal_plugin, "_resolve_managed_fal_video_gateway", lambda: object() + ) + monkeypatch.setattr( + fal_plugin, "_get_managed_fal_video_client", lambda gateway: client + ) + + with pytest.raises(ValueError) as exc_info: + fal_plugin._submit_fal_video_request("fal-ai/video", {"prompt": "x"}) + + message = str(exc_info.value) + assert "BILLING_ERROR" in message + assert "unsupported_pricing_meter" in message + assert "Nous Portal billing" in message + assert "may not yet be enabled" not in message + + def test_kling_4k_uses_start_image_url(): """Kling v3 4K's image-to-video endpoint expects start_image_url, not image_url. The family must declare image_param_key='start_image_url'.""" @@ -161,9 +209,10 @@ class TestFamilyRouting: fake.submit = _submit # type: ignore monkeypatch.setitem(sys.modules, "fal_client", fake) - # Reset the lazy global so it picks up our stub + # Pin the stub directly so the clean test environment does not attempt + # to auto-install the optional fal-client distribution. from plugins.video_gen import fal as fal_plugin - fal_plugin._fal_client = None + fal_plugin._fal_client = fake # Also reset the managed client cache fal_plugin._managed_fal_video_client = None fal_plugin._managed_fal_video_client_config = None @@ -532,7 +581,7 @@ class TestUpscalePass: monkeypatch.setitem(sys.modules, "fal_client", fake) from plugins.video_gen import fal as fal_plugin - fal_plugin._fal_client = None + fal_plugin._fal_client = fake fal_plugin._managed_fal_video_client = None fal_plugin._managed_fal_video_client_config = None diff --git a/tests/tools/test_fal_common.py b/tests/tools/test_fal_common.py index 5884982cf7..2d04232c48 100644 --- a/tests/tools/test_fal_common.py +++ b/tests/tools/test_fal_common.py @@ -317,6 +317,39 @@ class TestManagedFalSyncClientSubmit: client._request_handle_class.assert_called_once() assert result is client._request_handle_class.return_value + def test_submit_with_idempotency_key_is_not_retried(self): + """The Nous gateway cannot replay an accepted submit reliably. + + A retry can replace the original billing/entitlement response with a + misleading ``idempotency key ... without a reusable request handle`` + conflict, so keyed submits must make exactly one POST attempt. + """ + http_client = MagicMock() + response = MagicMock() + response.json.return_value = { + "request_id": "req-1", + "response_url": "https://q.example.com/resp", + "status_url": "https://q.example.com/status", + "cancel_url": "https://q.example.com/cancel", + } + http_client.request.return_value = response + client, _, _ = self._make_client(http_client=http_client) + client._maybe_retry_request = MagicMock() + client._raise_for_status = MagicMock() + client._request_handle_class = MagicMock() + + client.submit( + "my-app", {"prompt": "hello"}, + headers={"X-Idempotency-Key": "submission-1"}, + ) + + http_client.request.assert_called_once_with( + "POST", "https://queue.example.com/my-app", + json={"prompt": "hello"}, timeout=120.0, + headers={"X-Idempotency-Key": "submission-1"}, + ) + client._maybe_retry_request.assert_not_called() + def test_submit_with_path(self): client, _, _ = self._make_client() response = MagicMock() diff --git a/tests/tools/test_image_generation.py b/tests/tools/test_image_generation.py index 0fdf952101..da250fcd8b 100644 --- a/tests/tools/test_image_generation.py +++ b/tests/tools/test_image_generation.py @@ -393,15 +393,22 @@ class TestRegistryIntegration: # --------------------------------------------------------------------------- class _MockResponse: - def __init__(self, status_code: int): + def __init__(self, status_code: int, payload=None): self.status_code = status_code + self._payload = payload + self.text = "" if payload is None else __import__("json").dumps(payload) + + def json(self): + if self._payload is None: + raise ValueError("not json") + return self._payload class _MockHttpxError(Exception): """Simulates httpx.HTTPStatusError which exposes .response.status_code.""" - def __init__(self, status_code: int, message: str = "Bad Request"): + def __init__(self, status_code: int, message: str = "Bad Request", payload=None): super().__init__(message) - self.response = _MockResponse(status_code) + self.response = _MockResponse(status_code, payload) class TestExtractHttpStatus: @@ -423,6 +430,11 @@ class TestExtractHttpStatus: class TestManagedGatewayErrorTranslation: """4xx from the Nous managed gateway should be translated to a user-actionable message.""" + @pytest.fixture(autouse=True) + def _skip_optional_fal_dependency(self, image_tool, monkeypatch): + """These unit tests replace the managed client and need no FAL SDK.""" + monkeypatch.setattr(image_tool, "_load_fal_client", lambda: None) + def test_4xx_translates_to_value_error_with_remediation(self, image_tool, monkeypatch): """403 from managed gateway → ValueError mentioning FAL_KEY + hermes tools.""" from unittest.mock import MagicMock @@ -451,6 +463,49 @@ class TestManagedGatewayErrorTranslation: # Original exception chained for debugging assert exc_info.value.__cause__ is bad_request + def test_billing_meter_error_is_preserved_instead_of_called_model_unavailable( + self, image_tool, monkeypatch + ): + """Portal billing configuration is the root cause, not a missing model.""" + from unittest.mock import MagicMock + + managed_gateway = MagicMock() + managed_gateway.gateway_origin = "https://fal-queue-gateway.example.com" + managed_gateway.nous_user_token = "test-token" + monkeypatch.setattr( + image_tool, "_resolve_managed_fal_gateway", lambda: managed_gateway + ) + payload = { + "error": { + "code": "BILLING_ERROR", + "message": "Charge authorization failed", + "details": { + "upstreamPayload": { + "code": "unsupported_pricing_meter", + "error": "Unsupported resolver usage meter", + } + }, + } + } + billing_error = _MockHttpxError(409, payload=payload) + mock_managed_client = MagicMock() + mock_managed_client.submit.side_effect = billing_error + monkeypatch.setattr( + image_tool, "_get_managed_fal_client", lambda gw: mock_managed_client + ) + + with pytest.raises(ValueError) as exc_info: + image_tool._submit_fal_request( + "openai/gpt-image-2.5/flare/text-to-image", {"prompt": "x"} + ) + + msg = str(exc_info.value) + assert "Charge authorization failed" in msg + assert "BILLING_ERROR" in msg + assert "unsupported_pricing_meter" in msg + assert "Nous Portal billing" in msg + assert "may not yet be enabled" not in msg + def test_non_http_exception_from_managed_bubbles_up(self, image_tool, monkeypatch): """Connection errors, timeouts, etc. from managed mode aren't 4xx — diff --git a/tests/tools/test_managed_media_gateways.py b/tests/tools/test_managed_media_gateways.py index 8117e44fcd..8d4171fc71 100644 --- a/tests/tools/test_managed_media_gateways.py +++ b/tests/tools/test_managed_media_gateways.py @@ -74,6 +74,9 @@ def _install_fake_tools_package(): get_session_info=lambda: {}, ) ) + lazy_deps_module = types.ModuleType("tools.lazy_deps") + setattr(lazy_deps_module, "ensure", lambda *args, **kwargs: None) + sys.modules["tools.lazy_deps"] = lazy_deps_module sys.modules["tools.managed_tool_gateway"] = _load_tool_module( "tools.managed_tool_gateway", "managed_tool_gateway.py", @@ -93,7 +96,7 @@ def _install_fake_fal_client(captured): "cancel_url": "http://127.0.0.1:3009/requests/req-123/cancel", } - def _maybe_retry_request(client, method, url, json=None, timeout=None, headers=None): + def _capture_request(client, method, url, json=None, timeout=None, headers=None): captured["submit_via"] = "managed_client" captured["http_client"] = client captured["method"] = method @@ -103,6 +106,13 @@ def _install_fake_fal_client(captured): captured["headers"] = headers return FakeResponse() + def _maybe_retry_request(client, method, url, json=None, timeout=None, headers=None): + return _capture_request(client, method, url, json, timeout, headers) + + class FakeHttpClient: + def request(self, method, url, json=None, timeout=None, headers=None): + return _capture_request(self, method, url, json, timeout, headers) + class SyncRequestHandle: def __init__(self, request_id, response_url, status_url, cancel_url, client): captured["request_id"] = request_id @@ -117,7 +127,7 @@ def _install_fake_fal_client(captured): captured["client_key"] = key captured["client_timeout"] = default_timeout self.default_timeout = default_timeout - self._client = object() + self._client = FakeHttpClient() fal_client_module = types.SimpleNamespace( submit=submit, diff --git a/tests/tools/test_video_generation_tool_surface_matrix.py b/tests/tools/test_video_generation_tool_surface_matrix.py index 96f7e74f12..d32d9b08ce 100644 --- a/tests/tools/test_video_generation_tool_surface_matrix.py +++ b/tests/tools/test_video_generation_tool_surface_matrix.py @@ -59,6 +59,8 @@ def matrix_env(tmp_path, monkeypatch): fake_fal.submit = _submit # type: ignore monkeypatch.setitem(__import__("sys").modules, "fal_client", fake_fal) + import tools.fal_common as fal_common + monkeypatch.setattr(fal_common, "import_fal_client", lambda: fake_fal) # httpx stub for xAI import httpx @@ -100,11 +102,7 @@ def matrix_env(tmp_path, monkeypatch): async def _no_sleep(*a, **k): return None monkeypatch.setattr(asyncio, "sleep", _no_sleep) - # Reset FAL plugin's lazy fal_client cache so it picks up the stub - from plugins.video_gen import fal as fal_plugin - fal_plugin._fal_client = None - - # Force discovery + # Force discovery. from hermes_cli.plugins import _ensure_plugins_discovered _ensure_plugins_discovered(force=True) diff --git a/tools/fal_common.py b/tools/fal_common.py index c43881ef11..7670329dc2 100644 --- a/tools/fal_common.py +++ b/tools/fal_common.py @@ -46,6 +46,30 @@ def _extract_http_status(exc: BaseException) -> Optional[int]: return status if isinstance(status, int) else None +def _managed_fal_billing_error(exc: BaseException) -> Optional[Dict[str, str]]: + """Return normalized Nous billing details from a managed-gateway error.""" + response = getattr(exc, "response", None) + if response is None: + return None + try: + payload = response.json() + except Exception: # noqa: BLE001 — diagnostics must not mask the provider error + return None + error = payload.get("error") if isinstance(payload, dict) else None + if not isinstance(error, dict) or error.get("code") != "BILLING_ERROR": + return None + raw_details = error.get("details") + details = raw_details if isinstance(raw_details, dict) else {} + raw_upstream = details.get("upstreamPayload") + upstream = raw_upstream if isinstance(raw_upstream, dict) else {} + return { + "message": str(error.get("message") or "Charge authorization failed"), + "error_code": str(error.get("code") or "BILLING_ERROR"), + "code": str(upstream.get("code") or details.get("chargeIntentErrorCode") or "billing_error"), + "detail": str(upstream.get("error") or "Nous Portal rejected the charge authorization"), + } + + def _require(value: Any, what: str) -> Any: if value is None: raise RuntimeError(f"{what} is required for managed FAL gateway mode") @@ -93,9 +117,28 @@ class _ManagedFalSyncClient: if self._add_timeout_header is None: raise RuntimeError("fal_client.client.add_timeout_header is required for timeout requests") self._add_timeout_header(start_timeout, request_headers) - response = self._maybe_retry_request( - self._http_client, "POST", url, json=arguments, - timeout=getattr(self._sync_client, "default_timeout", 120.0), headers=request_headers) + request_kwargs = { + "json": arguments, + "timeout": getattr(self._sync_client, "default_timeout", 120.0), + "headers": request_headers, + } + # The Nous gateway currently records a keyed submission before billing + # authorization finishes, but cannot replay the resulting error. The + # SDK's automatic 409 retry therefore replaces the real billing error + # with an idempotency conflict. Make one attempt when the caller supplied + # a key; an ambiguous transport failure is safer than a possible duplicate + # generation or a masked entitlement failure. + has_idempotency_key = any( + str(key).lower() == "x-idempotency-key" for key in request_headers + ) + if has_idempotency_key: + response = self._http_client.request("POST", url, **request_kwargs) + else: + retry_request = self._maybe_retry_request + if retry_request is None: # guarded in __init__; keeps static analysis honest + raise RuntimeError("fal_client.client request helpers are required") + response = retry_request( + self._http_client, "POST", url, **request_kwargs) self._raise_for_status(response) data = response.json() return self._request_handle_class( diff --git a/tools/image_generation_tool.py b/tools/image_generation_tool.py index 853f2e44f9..328d4dcd14 100644 --- a/tools/image_generation_tool.py +++ b/tools/image_generation_tool.py @@ -29,7 +29,10 @@ def _load_fal_client() -> Any: from tools.debug_helpers import DebugSession -from tools.fal_common import _ManagedFalSyncClient, _extract_http_status, _normalize_fal_queue_url_format +from tools.fal_common import ( + _ManagedFalSyncClient, _extract_http_status, _managed_fal_billing_error, + _normalize_fal_queue_url_format, +) from tools.image_generation_catalog import ( DEFAULT_ASPECT_RATIO, DEFAULT_MODEL, FAL_MODELS, UPSCALER_CREATIVITY, UPSCALER_DEFAULT_PROMPT, UPSCALER_FACTOR, UPSCALER_GUIDANCE_SCALE, UPSCALER_MODEL, UPSCALER_NEGATIVE_PROMPT, @@ -131,6 +134,16 @@ def _submit_fal_request(model: str, arguments: Dict[str, Any]): # (allowlist miss, billing gate): give remediation instead of a raw httpx error. status = _extract_http_status(exc) if status is not None and 400 <= status < 500: + billing = _managed_fal_billing_error(exc) + if billing is not None: + raise ValueError( + f"Nous Subscription gateway rejected model '{model}' (HTTP {status}): " + f"{billing['message']} ({billing['error_code']}; " + f"{billing['code']}: {billing['detail']}). " + "This is a Nous Portal billing configuration issue, not a missing local API key. " + "The managed route cannot run this model until Nous enables its billing meter; " + "a direct FAL_KEY is an optional bypass." + ) from exc gateway_message = "" if status in {401, 402, 403}: gateway_message = "\n\n" + nous_tool_gateway_unavailable_message(