fix(diagnostics): make optional failures observable and safe
This commit is contained in:
@@ -368,7 +368,7 @@ def _main_loop(stdscr: "curses._CursesWindow") -> None: # type: ignore # pragm
|
||||
elif key in (curses.KEY_UP, ord("k")):
|
||||
selected = max(selected - 1, 0)
|
||||
elif key in (curses.KEY_ENTER, 10, 13):
|
||||
_detail_view(stdscr, qp, green_attr=green_attr)
|
||||
status_line = _detail_view(stdscr, qp, green_attr=green_attr)
|
||||
elif key == ord("a"):
|
||||
try:
|
||||
status_line = _approve_from_tui(stdscr, qp)
|
||||
@@ -456,7 +456,7 @@ def _detail_view(
|
||||
qp: QueuedProposal,
|
||||
*,
|
||||
green_attr: int = 0,
|
||||
) -> None: # pragma: no cover
|
||||
) -> str: # pragma: no cover
|
||||
"""Render the full proposal. Scrollable. Press q to return."""
|
||||
lines = _detail_lines(qp, green_attr=green_attr)
|
||||
offset = 0
|
||||
@@ -473,7 +473,7 @@ def _detail_view(
|
||||
stdscr.refresh()
|
||||
key = stdscr.getch()
|
||||
if key in (ord("q"), 27):
|
||||
return
|
||||
return ""
|
||||
if key in (curses.KEY_DOWN, ord("j")):
|
||||
offset = min(offset + 1, max(0, len(lines) - 1))
|
||||
elif key in (curses.KEY_UP, ord("k")):
|
||||
@@ -484,28 +484,28 @@ def _detail_view(
|
||||
offset = max(0, len(lines) - 1)
|
||||
elif key == ord("a"):
|
||||
try:
|
||||
_approve_from_tui(stdscr, qp)
|
||||
except ApplyError:
|
||||
pass
|
||||
return
|
||||
return _approve_from_tui(stdscr, qp)
|
||||
except ApplyError as exc:
|
||||
return f"apply failed: {exc}"
|
||||
elif key == ord("m"):
|
||||
if qp.proposal.tool in _REPORT_ONLY_TOOLS:
|
||||
return
|
||||
return f"modify unavailable for {qp.proposal.tool}"
|
||||
edited = _modify(stdscr, qp)
|
||||
if edited is not None:
|
||||
try:
|
||||
_approve_from_tui(
|
||||
stdscr, qp, final_file=edited,
|
||||
notes="operator modified before approving",
|
||||
)
|
||||
except ApplyError:
|
||||
pass
|
||||
return
|
||||
if edited is None:
|
||||
return "modify aborted (no change)"
|
||||
try:
|
||||
return _approve_from_tui(
|
||||
stdscr, qp, final_file=edited,
|
||||
notes="operator modified before approving",
|
||||
)
|
||||
except ApplyError as exc:
|
||||
return f"apply failed: {exc}"
|
||||
elif key == ord("r"):
|
||||
reason = _prompt(stdscr, "reject reason: ")
|
||||
if reason:
|
||||
reject(qp, reason=reason)
|
||||
return
|
||||
return f"rejected {qp.proposal.tool} for [{qp.label}]"
|
||||
return "reject aborted (empty reason)"
|
||||
|
||||
|
||||
def _modify(stdscr: "curses._CursesWindow", qp: QueuedProposal) -> str | None: # type: ignore # pragma: no cover
|
||||
|
||||
+32
-6
@@ -16,6 +16,8 @@ import os
|
||||
import sys
|
||||
from typing import Any, Optional
|
||||
|
||||
from ..log import debug
|
||||
|
||||
|
||||
def filter_multiselect(
|
||||
items: list[str],
|
||||
@@ -42,7 +44,11 @@ def filter_multiselect(
|
||||
|
||||
try:
|
||||
tty_fd = open(tty_path, "r+b", buffering=0)
|
||||
except OSError:
|
||||
except OSError as exc:
|
||||
debug(
|
||||
"multi-select unavailable; treating it as cancellation",
|
||||
context={"error_type": type(exc).__name__, "tty": tty_path},
|
||||
)
|
||||
return None
|
||||
|
||||
try:
|
||||
@@ -73,7 +79,11 @@ def filter_select(
|
||||
|
||||
try:
|
||||
tty_fd = open(tty_path, "r+b", buffering=0)
|
||||
except OSError:
|
||||
except OSError as exc:
|
||||
debug(
|
||||
"filter-select unavailable; treating it as cancellation",
|
||||
context={"error_type": type(exc).__name__, "tty": tty_path},
|
||||
)
|
||||
return None
|
||||
|
||||
try:
|
||||
@@ -129,7 +139,11 @@ def _run_picker(items: list[str], *, title: str, tty_fd: int) -> Optional[str]:
|
||||
curses.nocbreak()
|
||||
curses.echo()
|
||||
curses.endwin()
|
||||
except Exception: # noqa: W0718 — curses can raise many error types
|
||||
except Exception as exc: # noqa: W0718 — curses can raise many error types
|
||||
debug(
|
||||
"filter-select display failed; treating it as cancellation",
|
||||
context={"error_type": type(exc).__name__},
|
||||
)
|
||||
return None
|
||||
finally:
|
||||
sys.__stdin__ = orig_stdin # type: ignore[assignment]
|
||||
@@ -292,7 +306,11 @@ def _run_multiselect(
|
||||
curses.nocbreak()
|
||||
curses.echo()
|
||||
curses.endwin()
|
||||
except Exception: # noqa: W0718
|
||||
except Exception as exc: # noqa: W0718
|
||||
debug(
|
||||
"multi-select display failed; treating it as cancellation",
|
||||
context={"error_type": type(exc).__name__},
|
||||
)
|
||||
return None
|
||||
finally:
|
||||
sys.__stdin__ = orig_stdin # type: ignore[assignment]
|
||||
@@ -558,13 +576,21 @@ def name_color_modal(
|
||||
"""
|
||||
try:
|
||||
tty_fd = open(tty_path, "r+b", buffering=0) # pylint: disable=consider-using-with
|
||||
except OSError:
|
||||
except OSError as exc:
|
||||
debug(
|
||||
"name/color picker unavailable; using defaults",
|
||||
context={"error_type": type(exc).__name__, "tty": tty_path},
|
||||
)
|
||||
return default_label, ""
|
||||
|
||||
try:
|
||||
fd_dup = os.dup(tty_fd.fileno())
|
||||
return _run_name_color(default_label, tty_fd=fd_dup, disclaimer=disclaimer)
|
||||
except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught
|
||||
except Exception as exc: # noqa: BLE001 # pylint: disable=broad-exception-caught
|
||||
debug(
|
||||
"name/color picker failed; using defaults",
|
||||
context={"error_type": type(exc).__name__},
|
||||
)
|
||||
return default_label, ""
|
||||
finally:
|
||||
tty_fd.close()
|
||||
|
||||
@@ -4,6 +4,7 @@ from __future__ import annotations
|
||||
|
||||
import typing
|
||||
|
||||
from ...log import debug
|
||||
from .types import Config
|
||||
|
||||
|
||||
@@ -52,7 +53,11 @@ def resolve_client_config(
|
||||
) -> Config:
|
||||
try:
|
||||
policy = resolver.resolve(client_ip, identity_token)
|
||||
except Exception: # noqa: BLE001 - a policy lookup failure must deny
|
||||
except Exception as exc: # noqa: BLE001 - a policy lookup failure must deny
|
||||
debug(
|
||||
"egress policy resolution failed; applying deny-all",
|
||||
context={"error_type": type(exc).__name__},
|
||||
)
|
||||
return Config(routes=(), deny_reason=DENY_RESOLVER_ERROR)
|
||||
return _config_from_policy(policy)
|
||||
|
||||
@@ -63,6 +68,10 @@ def resolve_client_context(
|
||||
try:
|
||||
policy, bottle_id, tokens = resolver.resolve_policy_and_bottle_id(
|
||||
client_ip, identity_token)
|
||||
except Exception: # noqa: BLE001 - a policy lookup failure must deny
|
||||
except Exception as exc: # noqa: BLE001 - a policy lookup failure must deny
|
||||
debug(
|
||||
"egress context resolution failed; applying deny-all",
|
||||
context={"error_type": type(exc).__name__},
|
||||
)
|
||||
return Config(routes=(), deny_reason=DENY_RESOLVER_ERROR), "", {}
|
||||
return _config_from_policy(policy), (bottle_id or ""), tokens
|
||||
|
||||
@@ -18,6 +18,7 @@ import urllib.request
|
||||
from collections.abc import Iterable
|
||||
from dataclasses import dataclass
|
||||
|
||||
from ..log import debug
|
||||
from ..orchestrator_auth import ROLE_CLI
|
||||
from ..trust_domain import CONTROL_PLANE
|
||||
from .server import ORCHESTRATOR_AUTH_HEADER
|
||||
@@ -53,6 +54,23 @@ class RegisteredBottle:
|
||||
env_var_secret: str = ""
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class BackendProbeFailure:
|
||||
"""Safe diagnostic for an optional backend discovery probe."""
|
||||
|
||||
backend: str
|
||||
error_type: str
|
||||
|
||||
|
||||
def _probe_failure(backend: str, exc: BaseException) -> BackendProbeFailure:
|
||||
failure = BackendProbeFailure(backend, type(exc).__name__)
|
||||
debug(
|
||||
"orchestrator discovery probe unavailable",
|
||||
context={"backend": failure.backend, "error_type": failure.error_type},
|
||||
)
|
||||
return failure
|
||||
|
||||
|
||||
class OrchestratorClient:
|
||||
"""Trusted host-side client for the orchestrator control plane.
|
||||
|
||||
@@ -245,32 +263,41 @@ def discover_orchestrator_url(*, timeout: float = 2.0) -> str:
|
||||
orchestrator TAP. Returns the first that answers `/health`; raises if none
|
||||
do (no orchestrator up — launch a bottle first)."""
|
||||
candidates: list[str] = []
|
||||
failures: list[BackendProbeFailure] = []
|
||||
try: # docker: loopback-published control plane
|
||||
from .lifecycle import DEFAULT_PORT as _DOCKER_PORT
|
||||
candidates.append(f"http://127.0.0.1:{_DOCKER_PORT}")
|
||||
except Exception: # noqa: BLE001 — backend optional
|
||||
except Exception as exc: # noqa: BLE001 — backend optional
|
||||
failures.append(_probe_failure("docker", exc))
|
||||
candidates.append("http://127.0.0.1:8099")
|
||||
try: # firecracker: infra VM control plane on the orchestrator TAP
|
||||
from ..backend.firecracker import netpool
|
||||
from ..backend.firecracker.infra_vm import ORCHESTRATOR_PORT
|
||||
candidates.append(
|
||||
f"http://{netpool.orch_slot().guest_ip}:{ORCHESTRATOR_PORT}")
|
||||
except Exception: # noqa: BLE001 — backend optional / not firecracker
|
||||
pass
|
||||
except Exception as exc: # noqa: BLE001 — backend optional / not firecracker
|
||||
failures.append(_probe_failure("firecracker", exc))
|
||||
try: # macOS: orchestrator container on its host-only address
|
||||
from ..backend.macos_container.infra import probe_orchestrator_url
|
||||
url = probe_orchestrator_url()
|
||||
if url:
|
||||
candidates.append(url)
|
||||
except Exception: # noqa: BLE001 — backend optional / not macOS
|
||||
pass
|
||||
except Exception as exc: # noqa: BLE001 — backend optional / not macOS
|
||||
failures.append(_probe_failure("macos-container", exc))
|
||||
for url in candidates:
|
||||
if OrchestratorClient(url, timeout=timeout).health():
|
||||
return url
|
||||
detail = ""
|
||||
if failures:
|
||||
detail = "; optional probes unavailable: " + ", ".join(
|
||||
f"{failure.backend} ({failure.error_type})" for failure in failures
|
||||
)
|
||||
raise OrchestratorClientError(
|
||||
"no running orchestrator control plane found (tried "
|
||||
+ ", ".join(candidates)
|
||||
+ "); launch a bottle first"
|
||||
+ ")"
|
||||
+ detail
|
||||
+ "; launch a bottle first"
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from ..log import debug
|
||||
from .client import OrchestratorClient, OrchestratorClientError
|
||||
|
||||
|
||||
@@ -27,7 +28,14 @@ def reprovision_bottles(
|
||||
try:
|
||||
if client.reprovision_gateway(bottle_id, secret):
|
||||
restored += 1
|
||||
except OrchestratorClientError:
|
||||
except OrchestratorClientError as exc:
|
||||
debug(
|
||||
"gateway secret reprovision failed; continuing with other bottles",
|
||||
context={
|
||||
"bottle_id": bottle_id,
|
||||
"error_type": type(exc).__name__,
|
||||
},
|
||||
)
|
||||
continue
|
||||
return restored
|
||||
|
||||
|
||||
@@ -379,9 +379,15 @@ class Handler(http.server.BaseHTTPRequestHandler):
|
||||
status, payload = dispatch(
|
||||
server.orchestrator, method, self.path, body, role=role)
|
||||
except Exception as e: # noqa: BLE001 — the control plane must stay up
|
||||
sys.stderr.write(f"orchestrator: {method} {self.path} failed: {e!r}\n")
|
||||
# Do not echo exception messages to the caller or logs: broker and
|
||||
# persistence exceptions can contain request data. The operation,
|
||||
# route, and exception type are enough to correlate a traceback.
|
||||
sys.stderr.write(
|
||||
f"orchestrator: {method} {self.path} failed "
|
||||
f"[error_type={type(e).__name__}]\n"
|
||||
)
|
||||
sys.stderr.flush()
|
||||
status, payload = 500, {"error": f"internal error: {e}"}
|
||||
status, payload = 500, {"error": "internal error"}
|
||||
data = json.dumps(payload).encode()
|
||||
self.send_response(status)
|
||||
self.send_header("Content-Type", "application/json")
|
||||
|
||||
@@ -34,6 +34,20 @@ class TestCliBackendBoundaries(unittest.TestCase):
|
||||
|
||||
|
||||
class TestRuntimeModuleSizes(unittest.TestCase):
|
||||
def test_no_runtime_module_grows_beyond_global_ceiling(self) -> None:
|
||||
"""A coarse ceiling catches new monoliths; focused caps stay tighter."""
|
||||
ceiling = 850
|
||||
oversized = [
|
||||
f"{path.relative_to(ROOT)} ({len(path.read_text().splitlines())})"
|
||||
for path in (ROOT / "bot_bottle").rglob("*.py")
|
||||
if len(path.read_text().splitlines()) > ceiling
|
||||
]
|
||||
self.assertEqual(
|
||||
[], oversized,
|
||||
f"runtime modules must stay at or below {ceiling} lines: "
|
||||
+ ", ".join(oversized),
|
||||
)
|
||||
|
||||
def test_egress_modules_stay_focused(self) -> None:
|
||||
caps = {
|
||||
"addon_core.py": 100,
|
||||
|
||||
@@ -48,12 +48,13 @@ class TestSharedReprovision(unittest.TestCase):
|
||||
client.reprovision_gateway.side_effect = [
|
||||
OrchestratorClientError("bad key"), True,
|
||||
]
|
||||
self.assertEqual(
|
||||
1,
|
||||
reprovision_bottles(
|
||||
with patch("bot_bottle.orchestrator.reprovision.debug") as debug:
|
||||
count = reprovision_bottles(
|
||||
client, {"10.0.0.1": "key-1", "10.0.0.2": "key-2"},
|
||||
),
|
||||
)
|
||||
)
|
||||
self.assertEqual(1, count)
|
||||
self.assertEqual("b1", debug.call_args.kwargs["context"]["bottle_id"])
|
||||
self.assertNotIn("bad key", repr(debug.call_args))
|
||||
|
||||
|
||||
class TestMacosReprovision(unittest.TestCase):
|
||||
|
||||
@@ -9,6 +9,7 @@ from __future__ import annotations
|
||||
|
||||
import unittest
|
||||
from typing import Any, Optional
|
||||
from unittest.mock import patch
|
||||
|
||||
from bot_bottle.cli.tui import _filter_items, _multiselect_loop, filter_multiselect, filter_select
|
||||
|
||||
@@ -49,8 +50,10 @@ class TestFilterSelectEmptyItems(unittest.TestCase):
|
||||
|
||||
def test_returns_none_when_tty_unavailable(self):
|
||||
# /nonexistent is guaranteed to not open.
|
||||
result = filter_select(["a", "b"], tty_path="/nonexistent/tty")
|
||||
with patch("bot_bottle.cli.tui.debug") as debug:
|
||||
result = filter_select(["a", "b"], tty_path="/nonexistent/tty")
|
||||
self.assertIsNone(result)
|
||||
self.assertEqual("FileNotFoundError", debug.call_args.kwargs["context"]["error_type"])
|
||||
|
||||
|
||||
class TestFilterMultiselectEmptyItems(unittest.TestCase):
|
||||
@@ -60,8 +63,10 @@ class TestFilterMultiselectEmptyItems(unittest.TestCase):
|
||||
self.assertEqual([], result)
|
||||
|
||||
def test_returns_none_when_tty_unavailable(self):
|
||||
result = filter_multiselect(["a", "b"], tty_path="/nonexistent/tty")
|
||||
with patch("bot_bottle.cli.tui.debug") as debug:
|
||||
result = filter_multiselect(["a", "b"], tty_path="/nonexistent/tty")
|
||||
self.assertIsNone(result)
|
||||
self.assertEqual("FileNotFoundError", debug.call_args.kwargs["context"]["error_type"])
|
||||
|
||||
|
||||
class TestMultiselectLoopReordering(unittest.TestCase):
|
||||
|
||||
@@ -3,15 +3,16 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import unittest
|
||||
from unittest.mock import patch
|
||||
|
||||
from bot_bottle.gateway.egress.addon_core import (
|
||||
from bot_bottle.gateway.egress.context import (
|
||||
DENY_RESOLVER_ERROR,
|
||||
DENY_UNATTRIBUTED,
|
||||
DENY_UNPARSEABLE,
|
||||
decide,
|
||||
resolve_client_config,
|
||||
resolve_client_context,
|
||||
)
|
||||
from bot_bottle.gateway.egress.matching import decide
|
||||
from bot_bottle.gateway.policy_resolver import PolicyResolveError
|
||||
|
||||
|
||||
@@ -44,7 +45,13 @@ class TestResolveClientConfig(unittest.TestCase):
|
||||
|
||||
def test_resolver_error_denies_all(self) -> None:
|
||||
# Orchestrator unreachable/errored must never widen egress.
|
||||
self.assertEqual((), resolve_client_config(_FakeResolver(raises=True), "10.243.0.1").routes)
|
||||
with patch("bot_bottle.gateway.egress.context.debug") as debug:
|
||||
config = resolve_client_config(_FakeResolver(raises=True), "10.243.0.1")
|
||||
self.assertEqual((), config.routes)
|
||||
self.assertEqual(
|
||||
"PolicyResolveError", debug.call_args.kwargs["context"]["error_type"],
|
||||
)
|
||||
self.assertNotIn("orchestrator down", repr(debug.call_args))
|
||||
|
||||
def test_unparseable_policy_denies_all(self) -> None:
|
||||
cfg = resolve_client_config(_FakeResolver(result="routes: notalist\n"), "10.243.0.1")
|
||||
|
||||
@@ -12,7 +12,9 @@ from bot_bottle.orchestrator.client import (
|
||||
OrchestratorClient,
|
||||
OrchestratorClientError,
|
||||
RegisteredBottle,
|
||||
BackendProbeFailure,
|
||||
_host_auth_token,
|
||||
_probe_failure,
|
||||
)
|
||||
|
||||
_URLOPEN = "bot_bottle.orchestrator.client.urllib.request.urlopen"
|
||||
@@ -33,6 +35,15 @@ class TestHostAuthToken(unittest.TestCase):
|
||||
self.assertEqual("", _host_auth_token())
|
||||
|
||||
|
||||
class TestBackendProbeFailure(unittest.TestCase):
|
||||
def test_records_safe_typed_diagnostic(self) -> None:
|
||||
with patch("bot_bottle.orchestrator.client.debug") as debug:
|
||||
result = _probe_failure("firecracker", RuntimeError("secret detail"))
|
||||
self.assertEqual(BackendProbeFailure("firecracker", "RuntimeError"), result)
|
||||
rendered = repr(debug.call_args)
|
||||
self.assertNotIn("secret detail", rendered)
|
||||
|
||||
|
||||
def _resp(status: int, payload: object) -> MagicMock:
|
||||
m = MagicMock()
|
||||
inner = m.__enter__.return_value
|
||||
|
||||
@@ -7,6 +7,7 @@ server tests), plus one real-socket round-trip to prove the handler wiring.
|
||||
from __future__ import annotations
|
||||
|
||||
import base64
|
||||
import io
|
||||
import json
|
||||
import secrets
|
||||
import sqlite3
|
||||
@@ -17,7 +18,7 @@ import urllib.error
|
||||
import urllib.request
|
||||
from contextlib import closing
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from bot_bottle.orchestrator_auth import ROLE_CLI, ROLE_GATEWAY, mint
|
||||
from bot_bottle.orchestrator.broker import StubBroker
|
||||
@@ -283,6 +284,25 @@ class TestServerRoundTrip(unittest.TestCase):
|
||||
))
|
||||
self.assertEqual(reg["bottle_id"], attr["bottle_id"])
|
||||
|
||||
def test_internal_failure_is_contextual_but_redacted(self) -> None:
|
||||
orch = MagicMock()
|
||||
orch.registry.all.side_effect = RuntimeError("SENSITIVE request value")
|
||||
with patch("sys.stderr", io.StringIO()) as stderr:
|
||||
server = make_server(orch, "127.0.0.1", 0)
|
||||
self.addCleanup(server.server_close)
|
||||
thread = threading.Thread(target=server.serve_forever, daemon=True)
|
||||
thread.start()
|
||||
self.addCleanup(server.shutdown)
|
||||
host, port = server.server_address[0], server.server_address[1]
|
||||
with self.assertRaises(urllib.error.HTTPError) as raised:
|
||||
urllib.request.urlopen(f"http://{host}:{port}/bottles", timeout=5)
|
||||
payload = json.loads(raised.exception.read())
|
||||
output = stderr.getvalue()
|
||||
self.assertEqual({"error": "internal error"}, payload)
|
||||
self.assertIn("GET /bottles", output)
|
||||
self.assertIn("RuntimeError", output)
|
||||
self.assertNotIn("SENSITIVE", output)
|
||||
|
||||
|
||||
class TestOrchestratorAuth(unittest.TestCase):
|
||||
"""Role-scoped control-plane tokens (issue #400 / #469 review): every route
|
||||
|
||||
Reference in New Issue
Block a user