You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
classMinimaxAPIError(Exception):
"""Base exception for Minimax API errors."""passclassMinimaxRequestError(MinimaxAPIError):
"""Request related errors."""pass
MinimaxRequestError is a subclass of MinimaxAPIError, but the two represent different error categories:
MinimaxAPIError — problems talking to the remote API (auth, network, 5xx, etc.). Worth retrying.
MinimaxRequestError — client-side problems (missing required field, bad combination, etc.). Never worth retrying.
Because of the inheritance, every except MinimaxAPIError block in minimax_mcp/server.py also catches MinimaxRequestError. This makes it impossible for callers to handle "transient" vs "permanent" failures differently — and it complicates retry logic, metrics, and error budgets.
Impact
All tool functions return the same generic error message format for both transient and permanent failures, even though clients might want to retry one and not the other.
The PR test: expand coverage to server.py and __main__.py (#72) #87 test suite had to be written around the inheritance — e.g. with pytest.raises(MinimaxRequestError) works, but a hypothetical try/except MinimaxAPIError: retry() would also catch MinimaxRequestError, which is the wrong behavior.
Suggested fix
Two options, in order of preference:
Make them siblings under MinimaxError (rename the current base to MinimaxError or add a new neutral base). Both MinimaxAPIError and MinimaxRequestError should be siblings, not parent/child.
At minimum, update existing except MinimaxAPIError as e: blocks to except (MinimaxAPIError, MinimaxRequestError) as e: (or except MinimaxError as e: after the refactor) and document the categories in each tool's docstring.
Option 1 is cleaner and a one-file change.
Discovered via
Issue filed as a follow-up to PR #87 (test coverage). Tests pass either way, but the inheritance makes the error-handling story confusing for new contributors.
Summary
In
minimax_mcp/exceptions.py:MinimaxRequestErroris a subclass ofMinimaxAPIError, but the two represent different error categories:MinimaxAPIError— problems talking to the remote API (auth, network, 5xx, etc.). Worth retrying.MinimaxRequestError— client-side problems (missing required field, bad combination, etc.). Never worth retrying.Because of the inheritance, every
except MinimaxAPIErrorblock inminimax_mcp/server.pyalso catchesMinimaxRequestError. This makes it impossible for callers to handle "transient" vs "permanent" failures differently — and it complicates retry logic, metrics, and error budgets.Impact
with pytest.raises(MinimaxRequestError)works, but a hypotheticaltry/except MinimaxAPIError: retry()would also catchMinimaxRequestError, which is the wrong behavior.Suggested fix
Two options, in order of preference:
MinimaxError(rename the current base toMinimaxErroror add a new neutral base). BothMinimaxAPIErrorandMinimaxRequestErrorshould be siblings, not parent/child.except MinimaxAPIError as e:blocks toexcept (MinimaxAPIError, MinimaxRequestError) as e:(orexcept MinimaxError as e:after the refactor) and document the categories in each tool's docstring.Option 1 is cleaner and a one-file change.
Discovered via
Issue filed as a follow-up to PR #87 (test coverage). Tests pass either way, but the inheritance makes the error-handling story confusing for new contributors.
🤖 Generated with Claude Code