From 490892494516432e64b5df5e5c7aba727d7d68fd Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Thu, 7 May 2026 12:29:29 +0300 Subject: [PATCH] refactor(config): remove legacy unauth REDIS_URL fallbacks (#645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `config.REDIS_URL` is the canonical Redis URL gate (#589 / PR #643) — it raises at import if creds are missing. Three services bypassed that gate by reading the env var directly with an unauthenticated localhost fallback, both at the factory site and in the constructor default: redis_url = os.getenv("REDIS_URL", "redis://redis:6379") def __init__(self, redis_url: str = "redis://redis:6379"): In docker-compose these branches don't fire — REDIS_URL is always populated. But test/CI paths and ad-hoc debug shells silently fall back to unauth localhost, then hit NOAUTH at runtime instead of the clear startup-time error #589 establishes. Changes: - slot_service / ssh_service: factories drop the os.getenv fallback; constructors take Optional[str] = None and lazy-import config.REDIS_URL when called with no arg. - capacity_manager: same pattern, lazy-import in __init__. - tests/unit/conftest.py: setdefault REDIS_URL with creds before any backend import — unit tests don't share the parent conftest (norecursedirs = ..) so the env wasn't being primed. - tests/unit/test_redis_url_no_fallback.py: lint-style regression test that greps src/backend/services/ for `os.getenv("REDIS_URL"` and `"redis://redis:6379"` and fails if either resurfaces. Verified: - 30 unit tests pass in trinity-backend container (test_redis_url_no_fallback + test_capacity_manager + test_config_fail_fast) - 14 ssh_service tests + 9 redis-url-related tests pass on host venv - Live backend bootstraps cleanly: SlotService / SshService / CapacityManager all resolve REDIS_URL via config - Lint test catches regression: stashing the slot_service fix flips both assertions to FAIL with offending line:number Co-Authored-By: Claude Opus 4.7 (1M context) --- src/backend/services/capacity_manager.py | 3 +- src/backend/services/slot_service.py | 9 +-- src/backend/services/ssh_service.py | 8 ++- tests/unit/conftest.py | 10 +++ tests/unit/test_redis_url_no_fallback.py | 87 ++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 tests/unit/test_redis_url_no_fallback.py diff --git a/src/backend/services/capacity_manager.py b/src/backend/services/capacity_manager.py index 37eab13e9..6d28b5210 100644 --- a/src/backend/services/capacity_manager.py +++ b/src/backend/services/capacity_manager.py @@ -159,7 +159,8 @@ def __init__( slot_service: Optional[SlotService] = None, backlog_service: Optional[BacklogService] = None, ): - url = redis_url or os.getenv("REDIS_URL", "redis://redis:6379") + from config import REDIS_URL as _DEFAULT_REDIS_URL + url = redis_url or _DEFAULT_REDIS_URL self._redis = redis.from_url(url, decode_responses=True) self._slots = slot_service or SlotService(url) self._backlog = backlog_service or BacklogService() diff --git a/src/backend/services/slot_service.py b/src/backend/services/slot_service.py index 92b8c3d82..54d72ac76 100644 --- a/src/backend/services/slot_service.py +++ b/src/backend/services/slot_service.py @@ -63,7 +63,10 @@ class SlotService: - ZREMRANGEBYSCORE for cleanup """ - def __init__(self, redis_url: str = "redis://redis:6379"): + def __init__(self, redis_url: Optional[str] = None): + if redis_url is None: + from config import REDIS_URL + redis_url = REDIS_URL self.redis = redis.from_url(redis_url, decode_responses=True) self.slots_prefix = "agent:slots:" self.metadata_prefix = "agent:slot:" @@ -398,7 +401,5 @@ def get_slot_service() -> SlotService: """Get the global slot service instance.""" global _slot_service if _slot_service is None: - import os - redis_url = os.getenv("REDIS_URL", "redis://redis:6379") - _slot_service = SlotService(redis_url) + _slot_service = SlotService() # resolves REDIS_URL via config return _slot_service diff --git a/src/backend/services/ssh_service.py b/src/backend/services/ssh_service.py index d1c93d062..2d0ae8dc0 100644 --- a/src/backend/services/ssh_service.py +++ b/src/backend/services/ssh_service.py @@ -40,7 +40,10 @@ class SshService: """Service for managing ephemeral SSH access to agent containers.""" - def __init__(self, redis_url: str = "redis://redis:6379"): + def __init__(self, redis_url: Optional[str] = None): + if redis_url is None: + from config import REDIS_URL + redis_url = REDIS_URL self.redis_client = redis.from_url(redis_url, decode_responses=True) async def inject_ssh_key(self, agent_name: str, public_key: str) -> bool: @@ -546,6 +549,5 @@ def get_ssh_service() -> SshService: """Get the SSH service singleton.""" global _ssh_service if _ssh_service is None: - redis_url = os.getenv("REDIS_URL", "redis://redis:6379") - _ssh_service = redis_url and SshService(redis_url) + _ssh_service = SshService() # resolves REDIS_URL via config return _ssh_service diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 4c8446901..6835c0b08 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -4,12 +4,22 @@ These tests run without a backend connection (no Docker, no API). """ import importlib.util +import os import sys import types from pathlib import Path import pytest +# Issue #589 + #645: backend/config.py raises at import if REDIS_URL lacks +# credentials. Unit tests don't share the parent conftest (the unit/pytest.ini +# sets `norecursedirs = ..`), so set a creds-bearing dummy here. Any test +# that wants to assert the fail-fast behavior (e.g. test_config_fail_fast.py) +# still uses monkeypatch to override. +os.environ.setdefault("REDIS_URL", "redis://test:test@redis:6379") +os.environ.setdefault("REDIS_PASSWORD", "test") +os.environ.setdefault("REDIS_BACKEND_PASSWORD", "test") + # Ensure src/backend is importable before test modules are collected. # # BACKLOG-001: tests/unit/test_backlog.py does `from models import ...`, which diff --git a/tests/unit/test_redis_url_no_fallback.py b/tests/unit/test_redis_url_no_fallback.py new file mode 100644 index 000000000..f9f431549 --- /dev/null +++ b/tests/unit/test_redis_url_no_fallback.py @@ -0,0 +1,87 @@ +"""Lint-style regression for #645 (#589 follow-up). + +`config.py` raises at import if `REDIS_URL` lacks credentials (#589 / PR #643). +Three backend services bypassed that gate by reading the env var directly +with an unauthenticated localhost fallback: + + redis_url = os.getenv("REDIS_URL", "redis://redis:6379") + +This test enforces the single-source-of-truth principle: every backend +service must route Redis URL resolution through `config.REDIS_URL`. New +services that grow a `redis://` literal default — or a fresh +`os.getenv("REDIS_URL", ...)` fallback — trip these assertions. +""" + +from __future__ import annotations + +import os +import re +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.unit + + +def _backend_services_dir() -> Path: + """Locate src/backend/services across host and in-container layouts.""" + candidates = [ + Path(__file__).resolve().parent.parent.parent / "src" / "backend" / "services", + Path("/app/services"), # trinity-backend container + ] + env_override = os.environ.get("TRINITY_BACKEND_PATH") + if env_override: + candidates.insert(0, Path(env_override) / "services") + for c in candidates: + if c.is_dir(): + return c + raise RuntimeError( + "Cannot locate src/backend/services (set TRINITY_BACKEND_PATH)" + ) + + +_SERVICES = _backend_services_dir() +_PY_FILES = sorted(p for p in _SERVICES.rglob("*.py") if "__pycache__" not in p.parts) + + +def _grep(pattern: re.Pattern) -> list[tuple[Path, int, str]]: + """Return (path, lineno, line) for every match across the services tree.""" + hits: list[tuple[Path, int, str]] = [] + for path in _PY_FILES: + for lineno, line in enumerate(path.read_text().splitlines(), start=1): + if pattern.search(line): + hits.append((path, lineno, line.strip())) + return hits + + +# Direct env reads of REDIS_URL are forbidden — config.py is the canonical gate. +_GETENV_REDIS_URL = re.compile(r"""os\.getenv\s*\(\s*['"]REDIS_URL['"]""") + +# Unauthenticated localhost literals embedded as defaults are forbidden — they +# silently bypass #589's fail-fast guard. Constructors must default to None +# and resolve via config.REDIS_URL when called with no argument. +_UNAUTH_REDIS_LITERAL = re.compile(r"""['"]redis://redis:6379['"]""") + + +class TestNoOsGetenvRedisUrl: + def test_no_os_getenv_redis_url_under_services(self): + hits = _grep(_GETENV_REDIS_URL) + assert not hits, ( + "os.getenv(\"REDIS_URL\", ...) must not appear under " + "src/backend/services/. Import REDIS_URL from config instead " + "(see #645). Offending lines:\n" + + "\n".join(f" {p}:{n}: {s}" for p, n, s in hits) + ) + + +class TestNoUnauthRedisLiteralDefault: + def test_no_unauthenticated_redis_localhost_literal(self): + hits = _grep(_UNAUTH_REDIS_LITERAL) + assert not hits, ( + "'redis://redis:6379' literal must not appear under " + "src/backend/services/. It silently bypasses #589's fail-fast " + "credentials guard (see #645). Use Optional[str] = None + " + "lazy import of config.REDIS_URL inside the constructor. " + "Offending lines:\n" + + "\n".join(f" {p}:{n}: {s}" for p, n, s in hits) + )