mirror of
https://github.com/Significant-Gravitas/AutoGPT.git
synced 2026-04-30 03:00:41 -04:00
fix(util/service): preserve GraphValidationError.node_errors over RPC
``GraphValidationError`` carries a structured ``node_errors`` mapping
in addition to its top-level message, but RPC serialisation only
copied ``exc.args`` — so by the time the exception reached the
copilot client (for the schedule path, which goes via
``get_scheduler_client``), ``node_errors`` was ``{}``.
That broke the credential-race fallback introduced in the earlier
commits in this PR: the helper saw an empty ``node_errors``,
concluded it wasn't a credential error, and fell back to the generic
``ErrorResponse`` — exactly the symptom this PR is trying to fix.
Fix:
- Add an optional ``extras`` dict to ``RemoteCallError`` for
exception types that carry structured attributes beyond ``args``.
- In ``_handle_internal_http_error``, when the exception is a
``GraphValidationError``, pack ``node_errors`` into ``extras``.
- In ``_handle_call_method_response``, when reconstructing a
``GraphValidationError``, read ``extras.node_errors`` and pass it
to the constructor.
- Add two tests for the round-trip (with and without extras), the
latter to ensure backwards compatibility with old server responses.
This commit is contained in:
@@ -159,6 +159,13 @@ class BaseAppService(AppProcess, ABC):
|
||||
class RemoteCallError(BaseModel):
|
||||
type: str = "RemoteCallError"
|
||||
args: Optional[Tuple[Any, ...]] = None
|
||||
# Optional extras for exception types that carry structured attributes
|
||||
# beyond ``exc.args``. When set, the client-side handler uses these to
|
||||
# reconstruct the exception with the original attributes.
|
||||
# Currently used by ``GraphValidationError.node_errors`` so the
|
||||
# copilot's credential-race fallback can distinguish credential
|
||||
# failures from other graph validation errors over RPC.
|
||||
extras: Optional[dict[str, Any]] = None
|
||||
|
||||
|
||||
class UnhealthyServiceError(ValueError):
|
||||
@@ -238,11 +245,21 @@ class AppService(BaseAppService, ABC):
|
||||
f"{request.method} {request.url.path} failed: {exc}",
|
||||
exc_info=exc,
|
||||
)
|
||||
extras: Optional[dict[str, Any]] = None
|
||||
if isinstance(exc, exceptions.GraphValidationError):
|
||||
# ``exc.args`` only preserves the top-level message; the
|
||||
# structured ``node_errors`` mapping needs to ride along
|
||||
# in ``extras`` so the client can rebuild the original
|
||||
# exception state (used by the copilot credential-race
|
||||
# fallback to distinguish credential failures from other
|
||||
# validation errors).
|
||||
extras = {"node_errors": dict(exc.node_errors)}
|
||||
return responses.JSONResponse(
|
||||
status_code=status_code,
|
||||
content=RemoteCallError(
|
||||
type=str(exc.__class__.__name__),
|
||||
args=exc.args or (str(exc),),
|
||||
extras=extras,
|
||||
).model_dump(),
|
||||
)
|
||||
|
||||
@@ -614,6 +631,19 @@ def get_service_client(
|
||||
msg = str(args[0]) if args else str(e)
|
||||
raise exception_class({"user_facing_error": {"message": msg}})
|
||||
|
||||
# GraphValidationError carries a structured ``node_errors``
|
||||
# attribute that ``exc.args`` alone doesn't preserve.
|
||||
# If the server included it in ``extras``, thread it
|
||||
# back into the reconstructed exception.
|
||||
if issubclass(exception_class, exceptions.GraphValidationError):
|
||||
msg = str(args[0]) if args else str(e)
|
||||
node_errors = (
|
||||
error_response.extras.get("node_errors")
|
||||
if error_response.extras
|
||||
else None
|
||||
)
|
||||
raise exception_class(msg, node_errors=node_errors)
|
||||
|
||||
raise exception_class(*args)
|
||||
|
||||
# Otherwise categorize by HTTP status code
|
||||
|
||||
@@ -12,6 +12,7 @@ from prisma.errors import DataError, UniqueViolationError
|
||||
from pydantic import TypeAdapter
|
||||
|
||||
from backend.data.model import User
|
||||
from backend.util.exceptions import GraphValidationError
|
||||
from backend.util.service import (
|
||||
AppService,
|
||||
AppServiceClient,
|
||||
@@ -489,6 +490,66 @@ class TestHTTPErrorRetryBehavior:
|
||||
assert hasattr(exc_info.value, "data")
|
||||
assert isinstance(exc_info.value.data, dict)
|
||||
|
||||
def test_graph_validation_error_preserves_node_errors(self):
|
||||
"""GraphValidationError carries a structured ``node_errors`` mapping
|
||||
in addition to its top-level message. The server-side error handler
|
||||
packs it into ``RemoteCallError.extras`` and the client-side handler
|
||||
rebuilds the exception with ``node_errors`` preserved — without this
|
||||
round-trip the copilot's credential-race fallback can't distinguish
|
||||
credential failures from other validation errors, and users get a
|
||||
generic error instead of the inline credentials setup card.
|
||||
"""
|
||||
node_errors = {
|
||||
"some-node-id": {
|
||||
"credentials": "These credentials are required",
|
||||
"api_key": "Invalid credentials: not found",
|
||||
}
|
||||
}
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 400
|
||||
mock_response.json.return_value = {
|
||||
"type": "GraphValidationError",
|
||||
"args": ["Graph validation failed: 2 issues on 1 nodes"],
|
||||
"extras": {"node_errors": node_errors},
|
||||
}
|
||||
mock_response.raise_for_status.side_effect = httpx.HTTPStatusError(
|
||||
"400 Bad Request", request=Mock(), response=mock_response
|
||||
)
|
||||
|
||||
client = get_service_client(ServiceTestClient)
|
||||
|
||||
with pytest.raises(GraphValidationError) as exc_info:
|
||||
client._handle_call_method_response( # type: ignore[attr-defined]
|
||||
response=mock_response, method_name="test_method"
|
||||
)
|
||||
|
||||
assert "Graph validation failed" in str(exc_info.value)
|
||||
assert exc_info.value.node_errors == node_errors
|
||||
|
||||
def test_graph_validation_error_without_extras_still_deserializes(self):
|
||||
"""Backwards-compat: old server responses without ``extras`` should
|
||||
still reconstruct a ``GraphValidationError`` — just with an empty
|
||||
``node_errors`` mapping (matches current pre-fix behaviour)."""
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 400
|
||||
mock_response.json.return_value = {
|
||||
"type": "GraphValidationError",
|
||||
"args": ["Graph validation failed: 1 issues on 1 nodes"],
|
||||
}
|
||||
mock_response.raise_for_status.side_effect = httpx.HTTPStatusError(
|
||||
"400 Bad Request", request=Mock(), response=mock_response
|
||||
)
|
||||
|
||||
client = get_service_client(ServiceTestClient)
|
||||
|
||||
with pytest.raises(GraphValidationError) as exc_info:
|
||||
client._handle_call_method_response( # type: ignore[attr-defined]
|
||||
response=mock_response, method_name="test_method"
|
||||
)
|
||||
|
||||
assert "Graph validation failed" in str(exc_info.value)
|
||||
assert exc_info.value.node_errors == {}
|
||||
|
||||
def test_client_error_status_codes_coverage(self):
|
||||
"""Test that various 4xx status codes are all wrapped as HTTPClientError."""
|
||||
client_error_codes = [400, 401, 403, 404, 405, 409, 422, 429]
|
||||
|
||||
Reference in New Issue
Block a user