feat(supervise): typed RPC error taxonomy for dispatch
Introduce _RpcClientError and _RpcInternalError as distinct subclasses of _RpcError so the dispatcher can handle bad requests and server-side faults differently — returning client errors verbatim and logging internal faults with their cause before replying ERR_INTERNAL. Wrap write_proposal and archive_proposal IO with _RpcInternalError so OS failures surface through the typed path instead of the bare Exception fallback. All existing raise _RpcError(...) call sites converted to _RpcClientError. Closes #253
This commit is contained in:
@@ -90,19 +90,19 @@ def parse_jsonrpc(body: bytes) -> JsonRpcRequest:
|
||||
try:
|
||||
raw = json.loads(body)
|
||||
except json.JSONDecodeError as e:
|
||||
raise _RpcError(ERR_PARSE, f"parse error: {e}") from e
|
||||
raise _RpcClientError(ERR_PARSE, f"parse error: {e}") from e
|
||||
if not isinstance(raw, dict):
|
||||
raise _RpcError(ERR_INVALID_REQUEST, "request must be a JSON object")
|
||||
raise _RpcClientError(ERR_INVALID_REQUEST, "request must be a JSON object")
|
||||
if raw.get("jsonrpc") != JSONRPC_VERSION:
|
||||
raise _RpcError(ERR_INVALID_REQUEST, "jsonrpc field must be '2.0'")
|
||||
raise _RpcClientError(ERR_INVALID_REQUEST, "jsonrpc field must be '2.0'")
|
||||
method = raw.get("method")
|
||||
if not isinstance(method, str):
|
||||
raise _RpcError(ERR_INVALID_REQUEST, "method must be a string")
|
||||
raise _RpcClientError(ERR_INVALID_REQUEST, "method must be a string")
|
||||
params = raw.get("params", {})
|
||||
if params is None:
|
||||
params = {}
|
||||
if not isinstance(params, dict):
|
||||
raise _RpcError(ERR_INVALID_PARAMS, "params must be an object")
|
||||
raise _RpcClientError(ERR_INVALID_PARAMS, "params must be an object")
|
||||
rpc_id = raw.get("id", _NO_ID)
|
||||
is_notification = rpc_id is _NO_ID
|
||||
return JsonRpcRequest(
|
||||
@@ -117,12 +117,23 @@ _NO_ID = object()
|
||||
|
||||
|
||||
class _RpcError(Exception):
|
||||
"""Base class for all typed RPC errors that surface as JSON-RPC error responses."""
|
||||
def __init__(self, code: int, message: str):
|
||||
super().__init__(message)
|
||||
self.code = code
|
||||
self.message = message
|
||||
|
||||
|
||||
class _RpcClientError(_RpcError):
|
||||
"""Caller sent a bad request; returned verbatim, no server-side logging."""
|
||||
|
||||
|
||||
class _RpcInternalError(_RpcError):
|
||||
"""Server-side fault; logged at ERROR with cause, always returns ERR_INTERNAL."""
|
||||
def __init__(self, message: str) -> None:
|
||||
super().__init__(ERR_INTERNAL, message)
|
||||
|
||||
|
||||
def jsonrpc_result(request_id: object, result: object) -> bytes:
|
||||
payload = {"jsonrpc": JSONRPC_VERSION, "id": request_id, "result": result}
|
||||
return (json.dumps(payload) + "\n").encode("utf-8")
|
||||
@@ -290,7 +301,7 @@ def validate_proposed_file(tool: str, content: str) -> None:
|
||||
catches obvious paste-errors / wrong-tool selections before they
|
||||
enter the queue."""
|
||||
if not content.strip():
|
||||
raise _RpcError(ERR_INVALID_PARAMS, f"{tool}: proposed file is empty")
|
||||
raise _RpcClientError(ERR_INVALID_PARAMS, f"{tool}: proposed file is empty")
|
||||
if tool == _sv.TOOL_CAPABILITY_BLOCK:
|
||||
# Dockerfiles are too varied to validate syntactically beyond
|
||||
# non-empty. The operator reads the diff in the TUI.
|
||||
@@ -299,17 +310,17 @@ def validate_proposed_file(tool: str, content: str) -> None:
|
||||
try:
|
||||
config = load_config(content)
|
||||
except ValueError as e:
|
||||
raise _RpcError(
|
||||
raise _RpcClientError(
|
||||
ERR_INVALID_PARAMS,
|
||||
f"{tool}: proposed routes.yaml is not valid: {e}",
|
||||
) from e
|
||||
if config.log != LOG_OFF:
|
||||
raise _RpcError(
|
||||
raise _RpcClientError(
|
||||
ERR_INVALID_PARAMS,
|
||||
f"{tool}: proposed routes.yaml must not change egress logging",
|
||||
)
|
||||
else:
|
||||
raise _RpcError(ERR_INVALID_PARAMS, f"unknown tool {tool!r}")
|
||||
raise _RpcClientError(ERR_INVALID_PARAMS, f"unknown tool {tool!r}")
|
||||
|
||||
|
||||
# --- MCP handlers ----------------------------------------------------------
|
||||
@@ -382,17 +393,17 @@ def handle_tools_call(
|
||||
doesn't need operator approval."""
|
||||
name = params.get("name")
|
||||
if not isinstance(name, str):
|
||||
raise _RpcError(ERR_INVALID_PARAMS, "tools/call missing 'name'")
|
||||
raise _RpcClientError(ERR_INVALID_PARAMS, "tools/call missing 'name'")
|
||||
if name == _sv.TOOL_LIST_EGRESS_ROUTES:
|
||||
return handle_list_egress_routes(typing.cast(dict[str, object], params.get("arguments", {})), config)
|
||||
|
||||
args_raw = params.get("arguments", {})
|
||||
if not isinstance(args_raw, dict):
|
||||
raise _RpcError(ERR_INVALID_PARAMS, "tools/call 'arguments' must be an object")
|
||||
raise _RpcClientError(ERR_INVALID_PARAMS, "tools/call 'arguments' must be an object")
|
||||
|
||||
justification = args_raw.get("justification")
|
||||
if not isinstance(justification, str) or not justification.strip():
|
||||
raise _RpcError(
|
||||
raise _RpcClientError(
|
||||
ERR_INVALID_PARAMS,
|
||||
f"{name}: 'justification' is required and must be a non-empty string",
|
||||
)
|
||||
@@ -401,13 +412,13 @@ def handle_tools_call(
|
||||
file_field = PROPOSED_FILE_FIELD[name]
|
||||
proposed_file = args_raw.get(file_field)
|
||||
if not isinstance(proposed_file, str):
|
||||
raise _RpcError(
|
||||
raise _RpcClientError(
|
||||
ERR_INVALID_PARAMS,
|
||||
f"{name}: '{file_field}' is required and must be a string",
|
||||
)
|
||||
validate_proposed_file(name, proposed_file)
|
||||
else:
|
||||
raise _RpcError(ERR_INVALID_PARAMS, f"unknown tool {name!r}")
|
||||
raise _RpcClientError(ERR_INVALID_PARAMS, f"unknown tool {name!r}")
|
||||
|
||||
proposal = _sv.Proposal.new(
|
||||
bottle_slug=config.bottle_slug,
|
||||
@@ -416,7 +427,10 @@ def handle_tools_call(
|
||||
justification=justification,
|
||||
current_file_hash=_sv.sha256_hex(proposed_file),
|
||||
)
|
||||
_sv.write_proposal(config.queue_dir, proposal)
|
||||
try:
|
||||
_sv.write_proposal(config.queue_dir, proposal)
|
||||
except OSError as e:
|
||||
raise _RpcInternalError(f"failed to write proposal to queue: {e}") from e
|
||||
sys.stderr.write(
|
||||
f"supervise: queued proposal {proposal.id} ({name}) "
|
||||
f"for bottle {config.bottle_slug}; waiting for operator...\n"
|
||||
@@ -436,7 +450,10 @@ def handle_tools_call(
|
||||
"content": [{"type": "text", "text": text}],
|
||||
"isError": False,
|
||||
}
|
||||
_sv.archive_proposal(config.queue_dir, proposal.id)
|
||||
try:
|
||||
_sv.archive_proposal(config.queue_dir, proposal.id)
|
||||
except OSError as e:
|
||||
raise _RpcInternalError(f"failed to archive proposal: {e}") from e
|
||||
|
||||
text = format_response_text(response)
|
||||
return {
|
||||
@@ -512,7 +529,7 @@ class MCPHandler(http.server.BaseHTTPRequestHandler):
|
||||
|
||||
try:
|
||||
req = parse_jsonrpc(body)
|
||||
except _RpcError as e:
|
||||
except _RpcClientError as e:
|
||||
self._write_jsonrpc(jsonrpc_error(None, e.code, e.message))
|
||||
return
|
||||
|
||||
@@ -520,11 +537,19 @@ class MCPHandler(http.server.BaseHTTPRequestHandler):
|
||||
|
||||
try:
|
||||
result = self._dispatch(req, config)
|
||||
except _RpcError as e:
|
||||
except _RpcClientError as e:
|
||||
self._write_jsonrpc(jsonrpc_error(req.id, e.code, e.message))
|
||||
return
|
||||
except Exception as e: # noqa: W0718 — catch-all for RPC dispatch errors
|
||||
sys.stderr.write(f"supervise: internal error: {e}\n")
|
||||
except _RpcInternalError as e:
|
||||
cause = e.__cause__
|
||||
detail = f": {cause}" if cause else ""
|
||||
sys.stderr.write(f"supervise: internal error: {e.message}{detail}\n")
|
||||
sys.stderr.flush()
|
||||
self._write_jsonrpc(jsonrpc_error(req.id, ERR_INTERNAL, "internal error"))
|
||||
return
|
||||
except Exception as e: # noqa: W0718 — unexpected errors
|
||||
sys.stderr.write(f"supervise: unexpected error: {type(e).__name__}: {e}\n")
|
||||
sys.stderr.flush()
|
||||
self._write_jsonrpc(jsonrpc_error(req.id, ERR_INTERNAL, "internal error"))
|
||||
return
|
||||
|
||||
@@ -543,7 +568,7 @@ class MCPHandler(http.server.BaseHTTPRequestHandler):
|
||||
return handle_tools_list(req.params)
|
||||
if method == "tools/call":
|
||||
return handle_tools_call(req.params, config)
|
||||
raise _RpcError(ERR_METHOD_NOT_FOUND, f"method not found: {method}")
|
||||
raise _RpcClientError(ERR_METHOD_NOT_FOUND, f"method not found: {method}")
|
||||
|
||||
def _write_jsonrpc(self, body: bytes) -> None:
|
||||
self.send_response(200)
|
||||
|
||||
@@ -20,6 +20,7 @@ import supervise as _sv # noqa: E402 # type: ignore
|
||||
|
||||
from bot_bottle import supervise_server # noqa: E402
|
||||
from bot_bottle.supervise_server import (
|
||||
ERR_INTERNAL,
|
||||
ERR_INVALID_PARAMS,
|
||||
ERR_INVALID_REQUEST,
|
||||
ERR_METHOD_NOT_FOUND,
|
||||
@@ -29,7 +30,9 @@ from bot_bottle.supervise_server import (
|
||||
PROPOSED_FILE_FIELD,
|
||||
ServerConfig,
|
||||
TOOL_DEFINITIONS,
|
||||
_RpcClientError,
|
||||
_RpcError,
|
||||
_RpcInternalError,
|
||||
_response_timeout_from_env,
|
||||
format_response_text,
|
||||
handle_initialize,
|
||||
@@ -77,6 +80,65 @@ class TestValidation(unittest.TestCase):
|
||||
self.assertIn("must not change egress logging", cm.exception.message)
|
||||
|
||||
|
||||
# --- Error taxonomy --------------------------------------------------------
|
||||
|
||||
|
||||
class TestRpcErrorTaxonomy(unittest.TestCase):
|
||||
def test_rpc_client_error_is_rpc_error(self):
|
||||
e = _RpcClientError(ERR_INVALID_PARAMS, "bad param")
|
||||
self.assertIsInstance(e, _RpcError)
|
||||
self.assertEqual(ERR_INVALID_PARAMS, e.code)
|
||||
self.assertEqual("bad param", e.message)
|
||||
|
||||
def test_rpc_internal_error_is_rpc_error(self):
|
||||
e = _RpcInternalError("disk full")
|
||||
self.assertIsInstance(e, _RpcError)
|
||||
self.assertEqual(ERR_INTERNAL, e.code)
|
||||
self.assertEqual("disk full", e.message)
|
||||
|
||||
def test_rpc_internal_error_preserves_cause(self):
|
||||
cause = OSError("no space left on device")
|
||||
try:
|
||||
raise _RpcInternalError("failed to write") from cause
|
||||
except _RpcInternalError as e:
|
||||
self.assertIs(cause, e.__cause__)
|
||||
|
||||
def test_parse_error_is_client_error(self):
|
||||
with self.assertRaises(_RpcClientError):
|
||||
parse_jsonrpc(b"{bad json")
|
||||
|
||||
def test_validation_error_is_client_error(self):
|
||||
with self.assertRaises(_RpcClientError):
|
||||
validate_proposed_file(_sv.TOOL_EGRESS_ALLOW, "routes: nope\n")
|
||||
|
||||
def test_unknown_tool_in_tools_call_is_client_error(self):
|
||||
config = ServerConfig(bottle_slug="dev", queue_dir=Path("/unused"))
|
||||
with self.assertRaises(_RpcClientError) as cm:
|
||||
handle_tools_call({"name": "no-such-tool", "arguments": {}}, config)
|
||||
self.assertEqual(ERR_INVALID_PARAMS, cm.exception.code)
|
||||
|
||||
|
||||
class TestRpcInternalErrorOnIoFailure(unittest.TestCase):
|
||||
def test_write_proposal_os_error_raises_internal(self):
|
||||
config = ServerConfig(
|
||||
bottle_slug="dev",
|
||||
queue_dir=Path("/dev/null/cannot-exist"),
|
||||
)
|
||||
with self.assertRaises(_RpcInternalError) as cm:
|
||||
handle_tools_call(
|
||||
{
|
||||
"name": _sv.TOOL_CAPABILITY_BLOCK,
|
||||
"arguments": {
|
||||
"dockerfile": "FROM python:3.13\n",
|
||||
"justification": "x",
|
||||
},
|
||||
},
|
||||
config,
|
||||
)
|
||||
self.assertEqual(ERR_INTERNAL, cm.exception.code)
|
||||
self.assertIsNotNone(cm.exception.__cause__)
|
||||
|
||||
|
||||
# --- JSON-RPC parsing ------------------------------------------------------
|
||||
|
||||
|
||||
@@ -469,6 +531,26 @@ class TestHttpEndToEnd(unittest.TestCase):
|
||||
)
|
||||
self.assertEqual(ERR_METHOD_NOT_FOUND, result["error"]["code"]) # type: ignore[index]
|
||||
|
||||
def test_internal_error_returns_err_internal_over_http(self):
|
||||
with patch.object(
|
||||
supervise_server._sv, "write_proposal",
|
||||
side_effect=OSError("disk full"),
|
||||
):
|
||||
result = self._post_jsonrpc({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 99,
|
||||
"method": "tools/call",
|
||||
"params": {
|
||||
"name": _sv.TOOL_CAPABILITY_BLOCK,
|
||||
"arguments": {
|
||||
"dockerfile": "FROM python:3.13\n",
|
||||
"justification": "x",
|
||||
},
|
||||
},
|
||||
})
|
||||
self.assertIn("error", result)
|
||||
self.assertEqual(ERR_INTERNAL, result["error"]["code"]) # type: ignore[index]
|
||||
|
||||
def test_health_endpoint(self):
|
||||
conn = http.client.HTTPConnection("127.0.0.1", self.port, timeout=5)
|
||||
try:
|
||||
|
||||
Reference in New Issue
Block a user