From 2039ef635fc521a6a2c46749d3203777c2588c3e Mon Sep 17 00:00:00 2001 From: codex Date: Sun, 26 Jul 2026 06:14:44 +0000 Subject: [PATCH] refactor: validate reconciliation inputs and neutralize CLI helpers --- bot_bottle/backend/docker/util.py | 15 +-------------- bot_bottle/backend/resolve_common.py | 4 ++-- bot_bottle/cli/commands/start.py | 17 ++++++++--------- bot_bottle/orchestrator/server.py | 18 ++++++++++++------ bot_bottle/util.py | 20 ++++++++++++++++++++ tests/unit/test_orchestrator_server.py | 22 ++++++++++++++++++---- 6 files changed, 61 insertions(+), 35 deletions(-) diff --git a/bot_bottle/backend/docker/util.py b/bot_bottle/backend/docker/util.py index b05cbd6e..0124c982 100644 --- a/bot_bottle/backend/docker/util.py +++ b/bot_bottle/backend/docker/util.py @@ -6,12 +6,12 @@ from __future__ import annotations import os from datetime import datetime, timezone -import re import shutil import subprocess from typing import Iterator from ...log import die, info +from ...util import slugify def run_docker( @@ -114,19 +114,6 @@ def docker_cp(src: str, dest: str) -> None: f"{(result.stderr or '').strip() or ''}") -_SLUG_RE = re.compile(r"[^a-z0-9]+") - - -def slugify(name: str) -> str: - """Lowercase, non-alnum runs → '-', trimmed. Dies on empty result.""" - if not name: - die("slugify: missing name") - slug = _SLUG_RE.sub("-", name.lower()).strip("-") - if not slug: - die(f"name '{name}' produced an empty slug; use alphanumeric characters") - return slug - - def build_image(ref: str, context: str, *, dockerfile: str = "") -> None: """Invokes `docker build` every call. Layer cache makes no-change rebuilds cheap; running every time means Dockerfile edits land diff --git a/bot_bottle/backend/resolve_common.py b/bot_bottle/backend/resolve_common.py index 7ea9c145..1f22efce 100644 --- a/bot_bottle/backend/resolve_common.py +++ b/bot_bottle/backend/resolve_common.py @@ -30,6 +30,7 @@ from ..log import die from ..manifest import Manifest, ManifestBottle from ..supervisor.plan import SupervisePlan from ..orchestrator.supervisor import Supervisor +from ..util import slugify from . import BottleSpec @@ -44,8 +45,7 @@ def mint_slug(spec: BottleSpec) -> str: if spec.identity: return spec.identity if spec.label: - from .docker import util as docker_mod - return docker_mod.slugify(spec.label) + return slugify(spec.label) return bottle_identity(spec.agent_name) diff --git a/bot_bottle/cli/commands/start.py b/bot_bottle/cli/commands/start.py index 2082f928..71f19776 100644 --- a/bot_bottle/cli/commands/start.py +++ b/bot_bottle/cli/commands/start.py @@ -25,12 +25,11 @@ from typing import Callable from ...agent_provider import get_provider, runtime_for from ...backend import ( Bottle, + BottlePlan, BottleSpec, enumerate_active_agents, get_bottle_backend, ) -from ...backend.docker import util as docker_mod -from ...backend.docker.bottle_plan import DockerBottlePlan from ...bottle_state import ( cleanup_state, is_preserved, @@ -40,7 +39,7 @@ from ...image_cache import StaleImageError from ...log import info, die from ...manifest import Manifest, ManifestIndex from ..constants import PROG -from ...util import read_tty_line +from ...util import read_tty_line, slugify from .. import tui @@ -257,10 +256,10 @@ def _uniquify_label_headless(label: str) -> str: logging the chosen label. Orchestrators fire-and-forget many bottles, so silently picking a free name beats erroring on every collision.""" active_slugs = {a.slug for a in enumerate_active_agents()} - if docker_mod.slugify(label) not in active_slugs: + if slugify(label) not in active_slugs: return label n = 2 - while docker_mod.slugify(f"{label}-{n}") in active_slugs: + while slugify(f"{label}-{n}") in active_slugs: n += 1 chosen = f"{label}-{n}" info(f"label '{label}' already in use; using '{chosen}'") @@ -274,11 +273,11 @@ def prepare_with_preflight( spec: BottleSpec, *, stage_dir: Path, - render_preflight: Callable[[DockerBottlePlan, str], None], + render_preflight: Callable[[BottlePlan, str], None], prompt_yes: Callable[[], bool], dry_run: bool = False, backend_name: str | None = None, -) -> tuple[DockerBottlePlan | None, str]: +) -> tuple[BottlePlan | None, str]: """Run `backend.prepare`, render the preflight summary via the injected callable, prompt y/N via the injected callable. @@ -405,7 +404,7 @@ def _resolve_unique_label(label: str, color: str) -> tuple[str, str]: in use among running bottles. Passes through unchanged when no collision is found on the first check.""" while True: - slug_candidate = docker_mod.slugify(label) + slug_candidate = slugify(label) active_slugs = {a.slug for a in enumerate_active_agents()} if slug_candidate not in active_slugs: return label, color @@ -432,7 +431,7 @@ def _select_image_policy() -> str | None: def _text_render_preflight(): - def _render(plan: DockerBottlePlan, backend_name: str) -> None: + def _render(plan: BottlePlan, backend_name: str) -> None: print(file=sys.stderr) print(f"backend: {backend_name}", file=sys.stderr) print(_manifest_to_yaml(plan.manifest), file=sys.stderr) diff --git a/bot_bottle/orchestrator/server.py b/bot_bottle/orchestrator/server.py index a18dac3f..cd36b55a 100644 --- a/bot_bottle/orchestrator/server.py +++ b/bot_bottle/orchestrator/server.py @@ -57,6 +57,7 @@ from __future__ import annotations import http.server import json +import math import os import socketserver import sys @@ -217,13 +218,18 @@ def dispatch( # pylint: disable=too-many-return-statements,too-many-branches raw_ips = data.get("live_source_ips") if not isinstance(raw_ips, list): return 400, {"error": "live_source_ips (list of strings) is required"} - live = [ip for ip in raw_ips if isinstance(ip, str) and ip] + if any(not isinstance(ip, str) or not ip for ip in raw_ips): + return 400, {"error": "live_source_ips must contain non-empty strings"} + live = raw_ips grace = data.get("grace_seconds") - kwargs = ( - {"grace_seconds": float(grace)} - if isinstance(grace, (int, float)) and not isinstance(grace, bool) - else {} - ) + kwargs: dict[str, float] = {} + if grace is not None: + if isinstance(grace, bool) or not isinstance(grace, (int, float)): + return 400, {"error": "grace_seconds must be a non-negative finite number"} + parsed_grace = float(grace) + if not math.isfinite(parsed_grace) or parsed_grace < 0: + return 400, {"error": "grace_seconds must be a non-negative finite number"} + kwargs["grace_seconds"] = parsed_grace return 200, {"reaped": orch.reconcile(live, **kwargs)} if method == "POST" and route == "/attribute": diff --git a/bot_bottle/util.py b/bot_bottle/util.py index 767e365f..595b2bc9 100644 --- a/bot_bottle/util.py +++ b/bot_bottle/util.py @@ -9,8 +9,11 @@ import difflib import hashlib import ipaddress import os +import re import sys +from .log import die + def sha256_hex(content: str) -> str: """Hex SHA-256 of a UTF-8 string.""" @@ -67,3 +70,20 @@ def expand_tilde(path: str) -> str: home = os.environ.get("HOME", "") return home + path[1:] return path + + +_SLUG_RE = re.compile(r"[^a-z0-9]+") + + +def slugify(name: str) -> str: + """Return a portable bottle identifier from a human-readable name. + + This is deliberately a root utility: names are part of the generic CLI + and state model, not a Docker container concern. + """ + if not name: + die("slugify: missing name") + slug = _SLUG_RE.sub("-", name.lower()).strip("-") + if not slug: + die(f"name '{name}' produced an empty slug; use alphanumeric characters") + return slug diff --git a/tests/unit/test_orchestrator_server.py b/tests/unit/test_orchestrator_server.py index 495cc998..a43aa312 100644 --- a/tests/unit/test_orchestrator_server.py +++ b/tests/unit/test_orchestrator_server.py @@ -647,10 +647,24 @@ class TestReconcileRoute(unittest.TestCase): self.assertEqual(200, status) self.assertEqual([], payload["reaped"]) - def test_non_string_entries_are_ignored(self) -> None: - dead = self._old("10.0.0.4") + def test_non_string_entries_are_rejected(self) -> None: status, payload = dispatch( self.orch, "POST", "/reconcile", _body({"live_source_ips": [None, 7, "10.0.0.9"]})) - self.assertEqual(200, status) - self.assertEqual([dead], payload["reaped"]) + self.assertEqual(400, status) + self.assertIn("live_source_ips", payload["error"]) + + def test_empty_live_source_ip_is_rejected(self) -> None: + status, payload = dispatch( + self.orch, "POST", "/reconcile", _body({"live_source_ips": [""]})) + self.assertEqual(400, status) + self.assertIn("live_source_ips", payload["error"]) + + def test_invalid_grace_seconds_is_rejected(self) -> None: + for value in (True, "30", -1, float("inf"), float("nan")): + with self.subTest(value=value): + status, payload = dispatch( + self.orch, "POST", "/reconcile", + _body({"live_source_ips": [], "grace_seconds": value})) + self.assertEqual(400, status) + self.assertIn("grace_seconds", payload["error"])