diff --git a/bot_bottle/cli/commands/supervise.py b/bot_bottle/cli/commands/supervise.py index cbb349f4..cd2c5b7d 100644 --- a/bot_bottle/cli/commands/supervise.py +++ b/bot_bottle/cli/commands/supervise.py @@ -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 diff --git a/bot_bottle/cli/tui.py b/bot_bottle/cli/tui.py index 03cca8e3..e522d5c0 100644 --- a/bot_bottle/cli/tui.py +++ b/bot_bottle/cli/tui.py @@ -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() diff --git a/bot_bottle/gateway/egress/context.py b/bot_bottle/gateway/egress/context.py index 630a0be3..48e7c515 100644 --- a/bot_bottle/gateway/egress/context.py +++ b/bot_bottle/gateway/egress/context.py @@ -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 diff --git a/bot_bottle/orchestrator/client.py b/bot_bottle/orchestrator/client.py index 9d51a5a5..ce032364 100644 --- a/bot_bottle/orchestrator/client.py +++ b/bot_bottle/orchestrator/client.py @@ -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" ) diff --git a/bot_bottle/orchestrator/reprovision.py b/bot_bottle/orchestrator/reprovision.py index e55ebea0..31d6b172 100644 --- a/bot_bottle/orchestrator/reprovision.py +++ b/bot_bottle/orchestrator/reprovision.py @@ -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 diff --git a/bot_bottle/orchestrator/server.py b/bot_bottle/orchestrator/server.py index cd36b55a..bead932b 100644 --- a/bot_bottle/orchestrator/server.py +++ b/bot_bottle/orchestrator/server.py @@ -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") diff --git a/tests/unit/test_architecture_guardrails.py b/tests/unit/test_architecture_guardrails.py index 6efb0397..d937d188 100644 --- a/tests/unit/test_architecture_guardrails.py +++ b/tests/unit/test_architecture_guardrails.py @@ -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, diff --git a/tests/unit/test_backend_secret_reprovision.py b/tests/unit/test_backend_secret_reprovision.py index 3c25092c..7d74e197 100644 --- a/tests/unit/test_backend_secret_reprovision.py +++ b/tests/unit/test_backend_secret_reprovision.py @@ -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): diff --git a/tests/unit/test_cli_tui.py b/tests/unit/test_cli_tui.py index 605e9502..7a9d5dc4 100644 --- a/tests/unit/test_cli_tui.py +++ b/tests/unit/test_cli_tui.py @@ -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): diff --git a/tests/unit/test_egress_multitenant.py b/tests/unit/test_egress_multitenant.py index 2aa5c869..1ab4dbd2 100644 --- a/tests/unit/test_egress_multitenant.py +++ b/tests/unit/test_egress_multitenant.py @@ -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") diff --git a/tests/unit/test_orchestrator_client.py b/tests/unit/test_orchestrator_client.py index aa04a3e4..5ee3a84c 100644 --- a/tests/unit/test_orchestrator_client.py +++ b/tests/unit/test_orchestrator_client.py @@ -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 diff --git a/tests/unit/test_orchestrator_server.py b/tests/unit/test_orchestrator_server.py index a43aa312..e2ecadb4 100644 --- a/tests/unit/test_orchestrator_server.py +++ b/tests/unit/test_orchestrator_server.py @@ -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