polish(deploy): split docker entrypoint migration step

deploy/docker/entrypoint.py

@@ -10,6 +10,14 @@ WebSocket connections at ``/v1/runner/tunnel`` and never spawns
 harness subprocesses on its own. Users run ``omnigent run … --server
 <url>`` on their own machine; that runner dials in.
 
+Importing this module has **no side effects**: configuration loading,
+DB migrations, store construction, and app building all live inside
+``build_app()`` / ``run_migrations()`` / ``main()``. Nothing connects
+to a database, reads config, or builds the app until ``main()`` runs —
+which the ``if __name__ == "__main__":`` block (i.e. the Docker
+``CMD ["python", "/app/entrypoint.py"]``) invokes. This keeps the
+module importable for testing / tooling without a live database.
+
 Configuration is via environment variables:
 
   DATABASE_URL          Required. SQLAlchemy URL. Both PaaS-style URLs
@@ -29,7 +37,12 @@ import logging
 import os
 import sys
 import traceback
+from dataclasses import dataclass
 from pathlib import Path
+from typing import TYPE_CHECKING, Any
+
+if TYPE_CHECKING:
+    from fastapi import FastAPI
 
 logging.basicConfig(level=logging.INFO, stream=sys.stderr, force=True)
 logger = logging.getLogger("omnigent-docker")
@@ -45,16 +58,57 @@ _DEFAULT_HOST = "0.0.0.0"
 _DEFAULT_PORT = "8000"
 _DEFAULT_ARTIFACT_DIR = "/data/artifacts"
 
-try:
-    import uvicorn
+
+@dataclass(frozen=True)
+class _ResolvedConfig:
+    """Configuration resolved before migrations and app construction."""
+
+    cfg: dict[str, Any]
+    database_url: str
+    artifact_dir: Path
+    host: str
+    port: int
+
+
+@dataclass(frozen=True)
+class _BuiltApp:
+    """The bind address resolved alongside the built app.
+
+    ``build_app`` resolves HOST/PORT as a side effect of wiring the app
+    (they feed ``detect_base_url`` for accounts mode), so it returns
+    them here for ``main()`` to hand to ``uvicorn.run`` — keeping a
+    single config-resolution pass.
+    """
+
+    app: FastAPI
+    host: str
+    port: int
+
+
+def run_migrations(database_url: str) -> None:
+    """Run the Alembic upgrade against ``database_url``.
+
+    The SQLAlchemy stores refuse to start on a stale schema, so this
+    runs before any store boots. Creates a throwaway engine, upgrades,
+    and disposes it.
+    """
+    import sqlalchemy
+
+    from omnigent.db.utils import _run_migrations as _run_alembic_upgrade
+
+    migration_engine = sqlalchemy.create_engine(database_url)
+    try:
+        _run_alembic_upgrade(migration_engine, database_url)
+    finally:
+        migration_engine.dispose()
+
+
+def _resolve_config() -> _ResolvedConfig:
+    """Load config and resolve startup settings before migrations run."""
 
     from omnigent.db.utils import normalize_database_url
-    from omnigent.runner.transports.ws_tunnel.limits import (
-        RUNNER_TUNNEL_MAX_MESSAGE_BYTES,
-    )
-    from omnigent.server.app import create_app
     from omnigent.server.paas_env import detect_base_url, resolve_bind_host
-    from omnigent.server.server_config import config_str_list, load_server_config
+    from omnigent.server.server_config import load_server_config
 
     # ── Configuration ────────────────────────────────────────
     # Non-secret settings come from a YAML config file (default
@@ -66,8 +120,8 @@ try:
 
     # DATABASE_URL is env-first (compose/PaaS inject it; it's a secret),
     # with `database_uri:` in the config as a fallback for self-managed DBs.
-    DATABASE_URL = os.environ.get("DATABASE_URL") or cfg.get("database_uri")
-    if not DATABASE_URL:
+    database_url = os.environ.get("DATABASE_URL") or cfg.get("database_uri")
+    if not database_url:
         raise RuntimeError(
             "DATABASE_URL is required (env), or set `database_uri:` in the server config. "
             "Accepted forms: "
@@ -76,26 +130,26 @@ try:
         )
     # Normalize PaaS-style URLs (postgres:// or postgresql://) to the
     # psycopg3 dialect specifier that SQLAlchemy requires.
-    DATABASE_URL = normalize_database_url(DATABASE_URL)
+    database_url = normalize_database_url(database_url)
 
     # App settings are config-first, env fallback, then the built-in default.
-    ARTIFACT_DIR = Path(
+    artifact_dir = Path(
         cfg.get("artifact_location") or os.environ.get("ARTIFACT_DIR") or _DEFAULT_ARTIFACT_DIR
     )
     # resolve_bind_host strips the bracketed IPv6 form some platforms inject
     # ("[::]") and coerces Railway's IPv6 wildcard to IPv4 (its edge is v4-only).
-    HOST = resolve_bind_host(
+    host = resolve_bind_host(
         cfg.get("host") or os.environ.get("HOST"), os.environ, default=_DEFAULT_HOST
     )
-    PORT = int(cfg.get("port") or os.environ.get("PORT") or _DEFAULT_PORT)
-    ARTIFACT_DIR.mkdir(parents=True, exist_ok=True)
+    port = int(cfg.get("port") or os.environ.get("PORT") or _DEFAULT_PORT)
+    artifact_dir.mkdir(parents=True, exist_ok=True)
 
     logger.info(
         "Config: HOST=%s PORT=%d DB=%s ARTIFACTS=%s",
-        HOST,
-        PORT,
-        DATABASE_URL.split("@", 1)[-1] if "@" in DATABASE_URL else DATABASE_URL,
-        ARTIFACT_DIR,
+        host,
+        port,
+        database_url.split("@", 1)[-1] if "@" in database_url else database_url,
+        artifact_dir,
     )
 
     # Containerized / remote deploys default to authenticated auth.
@@ -139,29 +193,40 @@ try:
         # ("${VAR:-}"), which setdefault would leave in place — defeating the default.
         if not os.environ.get("OMNIGENT_ACCOUNTS_COOKIE_SECRET"):
             os.environ["OMNIGENT_ACCOUNTS_COOKIE_SECRET"] = load_or_generate_cookie_secret(
-                ARTIFACT_DIR
+                artifact_dir
             )
         if not os.environ.get("OMNIGENT_ACCOUNTS_BASE_URL"):
             # Auto-detect the public URL from the PaaS env (Render / Railway /
             # Fly / HF Spaces) so a 1-click deploy needs zero manual config;
             # falls back to the bind address for local / Docker / EC2.
             os.environ["OMNIGENT_ACCOUNTS_BASE_URL"] = detect_base_url(
-                os.environ, host=HOST, port=PORT
+                os.environ, host=host, port=port
             )
 
-    # ── Migrations ───────────────────────────────────────────
-    # Alembic upgrade runs before the stores boot — the SQLAlchemy
-    # stores refuse to start on a stale schema.
+    return _ResolvedConfig(
+        cfg=cfg,
+        database_url=database_url,
+        artifact_dir=artifact_dir,
+        host=host,
+        port=port,
+    )
 
-    import sqlalchemy
 
-    from omnigent.db.utils import _run_migrations as _run_alembic_upgrade
+def build_app(resolved_config: _ResolvedConfig | None = None) -> _BuiltApp:
+    """Resolve config if needed, wire the stores, and build the app.
 
-    _migration_engine = sqlalchemy.create_engine(DATABASE_URL)
-    try:
-        _run_alembic_upgrade(_migration_engine, DATABASE_URL)
-    finally:
-        _migration_engine.dispose()
+    This function intentionally does not run migrations; ``main()`` runs
+    them explicitly after config resolution and before store construction.
+    """
+    from omnigent.server.app import create_app
+    from omnigent.server.server_config import config_str_list
+
+    if resolved_config is None:
+        resolved_config = _resolve_config()
+
+    cfg = resolved_config.cfg
+    database_url = resolved_config.database_url
+    artifact_dir = resolved_config.artifact_dir
 
     # ── Stores ───────────────────────────────────────────────
 
@@ -186,21 +251,21 @@ try:
 
     telemetry.init()
 
-    agent_store = SqlAlchemyAgentStore(DATABASE_URL)
-    file_store = SqlAlchemyFileStore(DATABASE_URL)
-    conversation_store = SqlAlchemyConversationStore(DATABASE_URL)
-    comment_store = SqlAlchemyCommentStore(DATABASE_URL)
-    permission_store = SqlAlchemyPermissionStore(DATABASE_URL)
-    host_store = HostStore(DATABASE_URL)
+    agent_store = SqlAlchemyAgentStore(database_url)
+    file_store = SqlAlchemyFileStore(database_url)
+    conversation_store = SqlAlchemyConversationStore(database_url)
+    comment_store = SqlAlchemyCommentStore(database_url)
+    permission_store = SqlAlchemyPermissionStore(database_url)
+    host_store = HostStore(database_url)
     # Fail startup loud on a malformed `sandbox:` section (an operator
     # typo should not surface as a runtime 502 on the first managed
     # session); the startup catch-all below logs it.
     sandbox_config = parse_sandbox_config(cfg.get("sandbox"))
-    artifact_store = LocalArtifactStore(str(ARTIFACT_DIR))
+    artifact_store = LocalArtifactStore(str(artifact_dir))
 
     agent_cache = AgentCache(
         artifact_store=artifact_store,
-        cache_dir=ARTIFACT_DIR / ".cache",
+        cache_dir=artifact_dir / ".cache",
     )
 
     init_runtime(
@@ -227,7 +292,7 @@ try:
     if isinstance(auth_provider, _UAP) and auth_provider._source == "accounts":
         from omnigent.server.accounts_store import SqlAlchemyAccountStore
 
-        account_store = SqlAlchemyAccountStore(DATABASE_URL)
+        account_store = SqlAlchemyAccountStore(database_url)
 
     app = create_app(
         agent_store=agent_store,
@@ -248,20 +313,49 @@ try:
         sandbox_config=sandbox_config,
     )
 
-    if __name__ == "__main__":
-        logger.info("Starting omnigent server on %s:%d", HOST, PORT)
-        uvicorn.run(
-            app,
-            host=HOST,
-            port=PORT,
-            ws_max_size=RUNNER_TUNNEL_MAX_MESSAGE_BYTES,
+    return _BuiltApp(app=app, host=resolved_config.host, port=resolved_config.port)
+
+
+def main() -> None:
+    """Boot the server: build the app and hand it to uvicorn.
+
+    Wraps the whole boot in the startup catch-all so any failure
+    (config, migrations, store wiring) lands in the container logs and
+    the process holds open briefly for log capture before exiting
+    non-zero — the orchestrator then restarts us.
+    """
+    try:
+        resolved_config = _resolve_config()
+
+        # ── Migrations ───────────────────────────────────────────
+        # Alembic upgrade runs before the stores boot — the SQLAlchemy
+        # stores refuse to start on a stale schema.
+        run_migrations(resolved_config.database_url)
+
+        resolved = build_app(resolved_config)
+
+        import uvicorn
+
+        from omnigent.runner.transports.ws_tunnel.limits import (
+            RUNNER_TUNNEL_MAX_MESSAGE_BYTES,
         )
 
-except Exception:  # noqa: BLE001 — startup catch-all so failures land in logs
-    logger.error("FATAL: omnigent server failed to start:\n%s", traceback.format_exc())
-    # Keep the process alive briefly so the container log capture has time
-    # to flush before the orchestrator restarts us.
-    import time
+        logger.info("Starting omnigent server on %s:%d", resolved.host, resolved.port)
+        uvicorn.run(
+            resolved.app,
+            host=resolved.host,
+            port=resolved.port,
+            ws_max_size=RUNNER_TUNNEL_MAX_MESSAGE_BYTES,
+        )
+    except Exception:  # noqa: BLE001 — startup catch-all so failures land in logs
+        logger.error("FATAL: omnigent server failed to start:\n%s", traceback.format_exc())
+        # Keep the process alive briefly so the container log capture has time
+        # to flush before the orchestrator restarts us.
+        import time  # deferred — keeps module inert
 
-    time.sleep(30)
-    sys.exit(1)
+        time.sleep(30)
+        sys.exit(1)
+
+
+if __name__ == "__main__":
+    main()

tests/deploy/test_docker_entrypoint_import.py

@@ -0,0 +1,81 @@
+"""Guard: importing the OSS Docker entrypoint has no side effects.
+
+The Docker image runs ``python /app/entrypoint.py`` (see
+``deploy/docker/Dockerfile``), so all of the boot work — config load,
+Alembic migrations, store construction, ``create_app`` — lives behind
+``main()`` and must not fire at import time. This test enforces that:
+the module must import cleanly with ``DATABASE_URL`` unset and without
+ever touching the database (``sqlalchemy.create_engine`` is wired to
+blow up if called during import).
+"""
+
+from __future__ import annotations
+
+import importlib
+import sys
+from typing import NoReturn
+
+import pytest
+
+_ENTRYPOINT_MODULE = "deploy.docker.entrypoint"
+_BOOT_MODULES = (
+    "fastapi",
+    "omnigent.db.utils",
+    "omnigent.runtime",
+    "omnigent.server.app",
+    "omnigent.server.server_config",
+    "omnigent.stores.agent_store.sqlalchemy_store",
+    "omnigent.stores.artifact_store.local",
+    "uvicorn",
+)
+
+
+@pytest.fixture
+def _fresh_entrypoint_import(monkeypatch: pytest.MonkeyPatch) -> list[str]:
+    """Force a from-scratch import of the entrypoint, DB-unset.
+
+    Drops any cached copy of the module, clears ``DATABASE_URL`` so the
+    import can't lean on an ambient one, and trip-wires
+    ``sqlalchemy.create_engine`` so any import-time DB access fails the
+    test loudly rather than silently connecting.
+    """
+    monkeypatch.delenv("DATABASE_URL", raising=False)
+    monkeypatch.delitem(sys.modules, _ENTRYPOINT_MODULE, raising=False)
+    for module_name in _BOOT_MODULES:
+        monkeypatch.delitem(sys.modules, module_name, raising=False)
+
+    import sqlalchemy
+
+    create_engine_calls: list[str] = []
+
+    def _no_engine_at_import(*args: object, **kwargs: object) -> NoReturn:
+        create_engine_calls.append(repr((args, kwargs)))
+        raise AssertionError(
+            "sqlalchemy.create_engine() must not be called while importing "
+            f"{_ENTRYPOINT_MODULE} — DB work belongs in main()/build_app()."
+        )
+
+    monkeypatch.setattr(sqlalchemy, "create_engine", _no_engine_at_import)
+    return create_engine_calls
+
+
+def test_entrypoint_imports_without_side_effects(
+    _fresh_entrypoint_import: list[str],
+) -> None:
+    # Importing must not raise (the old module-level code raised
+    # RuntimeError here because DATABASE_URL was unset) and must not
+    # have created an engine (the monkeypatched create_engine would
+    # have raised AssertionError).
+    module = importlib.import_module(_ENTRYPOINT_MODULE)
+
+    # The boot entry points exist and the module is inert until called.
+    assert callable(module.main)
+    assert callable(module.build_app)
+    assert callable(module.run_migrations)
+    # No app was built at import time.
+    assert not hasattr(module, "app")
+    assert _fresh_entrypoint_import == []
+    # Config, migrations, runtime/store wiring, and create_app all stay behind
+    # build_app()/main() rather than being imported or executed at module import.
+    for module_name in _BOOT_MODULES:
+        assert module_name not in sys.modules