From a20c83f4688384973be42ebd95b912f456b4f2f2 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 11:32:50 +0100 Subject: [PATCH 01/14] feat: a development loop in one command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `docsgpt up --native` installs services meant to outlive the shell. Development wants the opposite, and until now it meant three terminals from the guide: uvicorn, celery, and vite. `docsgpt dev` runs this checkout's API and worker as children of one terminal, both restarting when a file is saved, their output interleaved and labelled, and Ctrl-C stopping them together. `--ui` adds the Vite dev server, `--mock-llm` runs the bundled mock model so no API key is needed, and `--no-worker` leaves the worker to your editor's debugger. Celery has no reloader of its own, so the worker is wrapped in watchfiles when it is installed, and runs plain when it is not. Alongside it, the commands a dev loop keeps reaching for: - `docsgpt doctor` checks what usually breaks a new setup: PostgreSQL answering and its schema matching this version, Redis answering, a model provider being configured, and the port being free. - `docsgpt restart [api|worker]` bounces services without rewriting settings or rerunning migrations, which `down` plus `up` did. - `docsgpt logs -f` follows a native install instead of telling you to run `tail -f` yourself. - `docsgpt env set` applies itself to a running native install rather than asking you to run `docsgpt up` again to change one value. Two bugs found on the way, both older than this change: - `docsgpt api --reload` watched the working directory, which in a checkout is 178,425 files: .venv, node_modules, and the indexes/ and inputs/ the app writes to while ingesting, so the server restarted itself mid-request. It watches the package now — 1,217 files. - The VS Code "Flask Debugger" ran `flask run`, which serves only the WSGI app: /mcp, the SSE streams and artifact downloads 404 under it. The guide warned about this in prose while the debug config did it anyway. It runs uvicorn on the ASGI app now, like production. --- .vscode/launch.json | 51 ++-- .../Deploying/Development-Environment.mdx | 52 +++- docs/content/changelog.mdx | 11 + docsgpt/cli.py | 35 ++- docsgpt/deploy/commands.py | 234 +++++++++++++++++- docsgpt/deploy/dev.py | 214 ++++++++++++++++ tests/deploy/test_dev.py | 180 ++++++++++++++ tests/deploy/test_doctor.py | 167 +++++++++++++ tests/test_cli.py | 24 +- 9 files changed, 932 insertions(+), 36 deletions(-) create mode 100644 docsgpt/deploy/dev.py create mode 100644 tests/deploy/test_dev.py create mode 100644 tests/deploy/test_doctor.py diff --git a/.vscode/launch.json b/.vscode/launch.json index 30700f70..28d4fb57 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -2,39 +2,36 @@ "version": "0.2.0", "configurations": [ { - "name": "Frontend Debug (npm)", + "name": "Frontend (npm)", "type": "node-terminal", "request": "launch", "command": "npm run dev", "cwd": "${workspaceFolder}/frontend" }, { - "name": "Flask Debugger", - "type": "debugpy", - "request": "launch", - "module": "flask", - "env": { - "FLASK_APP": "docsgpt/app.py", - "PYTHONPATH": "${workspaceFolder}", - "FLASK_ENV": "development", - "FLASK_DEBUG": "1", - "FLASK_RUN_PORT": "7091", - "FLASK_RUN_HOST": "0.0.0.0" - - }, - "args": [ - "run", - "--no-debugger" - ], - "cwd": "${workspaceFolder}", + "name": "API (uvicorn)", + "type": "debugpy", + "request": "launch", + "module": "uvicorn", + "env": { + "PYTHONPATH": "${workspaceFolder}" + }, + "args": [ + "docsgpt.asgi:asgi_app", + "--host", + "127.0.0.1", + "--port", + "7091" + ], + "cwd": "${workspaceFolder}" }, { - "name": "Celery Debugger", + "name": "Celery worker", "type": "debugpy", "request": "launch", "module": "celery", "env": { - "PYTHONPATH": "${workspaceFolder}", + "PYTHONPATH": "${workspaceFolder}" }, "args": [ "-A", @@ -47,10 +44,10 @@ "cwd": "${workspaceFolder}" }, { - "name": "Dev Containers (Mongo + Redis)", + "name": "Dev services (Postgres + Redis)", "type": "node-terminal", "request": "launch", - "command": "docker compose -f deployment/docker-compose-dev.yaml up --build", + "command": "docker compose -f deployment/docker-compose-dev.yaml up", "cwd": "${workspaceFolder}" } ], @@ -58,9 +55,9 @@ { "name": "DocsGPT: Full Stack", "configurations": [ - "Frontend Debug (npm)", - "Flask Debugger", - "Celery Debugger" + "Frontend (npm)", + "API (uvicorn)", + "Celery worker" ], "presentation": { "group": "DocsGPT", @@ -68,4 +65,4 @@ } } ] -} \ No newline at end of file +} diff --git a/docs/content/Deploying/Development-Environment.mdx b/docs/content/Deploying/Development-Environment.mdx index 6c01879f..133c5531 100644 --- a/docs/content/Deploying/Development-Environment.mdx +++ b/docs/content/Deploying/Development-Environment.mdx @@ -119,7 +119,34 @@ To run the DocsGPT backend locally, you'll need to set up a Python environment a 5. **Run the Backend:** - For local development, run the ASGI composition under uvicorn. It serves the **whole** application, hot-reloads on source changes, and matches the production runtime: + One command runs the API and the worker from this checkout, each restarting when you save a file: + + ```bash + docsgpt dev + ``` + + Both run as children of that terminal, with their output interleaved and labelled, and Ctrl-C stops + them together. Useful flags: + + | Flag | What it does | + | --- | --- | + | `--ui` | also start the Vite dev server, so the whole app runs from one command | + | `--mock-llm` | run `scripts/mock_llm.py` and point DocsGPT at it, so no API key is needed | + | `--no-worker` | leave the worker to you, for instance when debugging it in your editor | + | `--no-reload` | do not restart anything on save | + | `--port` | serve the API somewhere other than 7091 | + + `docsgpt dev` is for a checkout. `docsgpt up --native`, by contrast, installs supervised services + that outlive the shell — see [Run it as services](/Deploying/Pip-Install#run-it-as-services-without-docker). + + + `docsgpt doctor` checks the things that usually break a new setup: whether PostgreSQL answers and + its schema matches this version, whether Redis answers, whether a model provider is configured, + and whether the port is free. Run it first when something does not start. + + + To run the two processes yourself instead, start the ASGI composition under uvicorn. It serves the + **whole** application, hot-reloads on source changes, and matches the production runtime: ```bash uvicorn docsgpt.asgi:asgi_app --host 0.0.0.0 --port 7091 --reload @@ -135,7 +162,7 @@ To run the DocsGPT backend locally, you'll need to set up a Python environment a But it serves **only** the WSGI Flask app and omits the native-async routes mounted on the ASGI shell in `docsgpt/asgi.py`: the `/mcp` FastMCP endpoint, the chat reconnect reader `GET /api/messages//events`, the notification stream `GET /api/events`, the remote-device command stream `GET /api/devices/sessions//events`, and artifact downloads `GET /api/artifacts//download`. Under `flask run` those paths return 404 — chat still works (`POST /stream` is a Flask route), but live notifications, stream auto-resume, paired devices and artifact downloads don't. Use `flask run` only when you don't need them. -6. **Start the Celery Worker:** +6. **Start the Celery Worker** (not needed if you used `docsgpt dev`)**:** Open a new terminal window (and activate your virtual environment if you used one). Start the Celery worker to handle background tasks: @@ -153,10 +180,14 @@ To run the DocsGPT backend locally, you'll need to set up a Python environment a **Running in Debugger (VSCode):** -For easier debugging, you can launch the Flask app and Celery worker directly from VSCode's debugger. +For easier debugging, you can launch the API and the Celery worker directly from VSCode's debugger. * Press Shift + Cmd + D (macOS) or Shift + Windows + D (Windows) to open the Run and Debug view. -* You should see configurations named "Flask" and "Celery". Select the desired configuration and click the "Start Debugging" button (green play icon). +* You should see configurations named "API (uvicorn)" and "Celery worker", and a compound "DocsGPT: Full Stack" that starts them with the frontend. Select one and click the "Start Debugging" button (green play icon). + +The API configuration runs the same ASGI app as production, so the routes mounted on the ASGI shell +work under the debugger. It deliberately runs without `--reload`: the reloader restarts the server in +a child process, which your breakpoints would not be attached to. ## 3. Start the Frontend @@ -207,3 +238,16 @@ To run the DocsGPT frontend locally, you'll need Node.js and npm (Node Package M This command will start the Vite development server. The frontend application will typically be accessible at [http://localhost:5173/](http://localhost:5173/). The terminal will display the exact URL where the frontend is running. With both the backend and frontend running, you should now have a fully functional DocsGPT development environment. You can access the application in your browser at [http://localhost:5173/](http://localhost:5173/) and start developing! + +## Working on two branches at once + +Each install keeps its own directory and its own services, so a second branch can run beside the +first as long as it gets its own port: + +```bash +docsgpt dev --port 7092 # a second checkout, second terminal +docsgpt up --native --dir ~/.docsgpt/review --port 7092 # or a second installed copy +``` + +A native install in another directory gets its own service names, so the two never write over each +other's units. `docsgpt status --dir ~/.docsgpt/review` reports on that one alone. diff --git a/docs/content/changelog.mdx b/docs/content/changelog.mdx index ab78c4ad..59fa4ae8 100644 --- a/docs/content/changelog.mdx +++ b/docs/content/changelog.mdx @@ -13,6 +13,17 @@ request, and [Upgrading](/upgrading) covers the steps an existing deployment has ## Unreleased +### A development loop in one command + +`docsgpt dev` runs this checkout's API and worker as children of one terminal, both restarting when +you save, with their output interleaved and Ctrl-C stopping them together. `--ui` adds the Vite dev +server and `--mock-llm` runs the bundled mock model, so a working loop needs no API key. +`docsgpt doctor` checks PostgreSQL, its schema version, Redis, the model provider and the port; +`docsgpt restart` bounces the services without touching settings; `docsgpt logs -f` now follows a +native install; and `docsgpt env set` applies itself to a running native install instead of asking +you to run `docsgpt up` again. See +[Setting up a development environment](/Deploying/Development-Environment). + ### Run DocsGPT without Docker `docsgpt up --native` runs the API and the worker as services on the machine itself, launchd on diff --git a/docsgpt/cli.py b/docsgpt/cli.py index 1ca2a187..d837642d 100644 --- a/docsgpt/cli.py +++ b/docsgpt/cli.py @@ -89,7 +89,17 @@ def _api(args: argparse.Namespace) -> int: if args.reload or sys.platform == "win32": import uvicorn - uvicorn.run("docsgpt.asgi:asgi_app", host=args.host, port=args.port, reload=args.reload) + from docsgpt.core.paths import package_dir + + # Watch the package, not the working directory: a checkout also holds .venv, node_modules + # and the data the app writes (indexes/, inputs/), which restarts the server mid-ingest. + uvicorn.run( + "docsgpt.asgi:asgi_app", + host=args.host, + port=args.port, + reload=args.reload, + reload_dirs=[str(package_dir())] if args.reload else None, + ) return 0 _gunicorn_application(_gunicorn_options(args.host, args.port, args.workers)).run() @@ -246,12 +256,33 @@ def _add_deploy_commands(commands) -> None: restore.add_argument("--timeout", type=int, default=300, help="seconds to wait for the API afterwards (default: 300)") + doctor = stack_command("doctor", "doctor", "check what this machine needs to run DocsGPT") + doctor.add_argument("--postgres-uri", help="check this database instead of the one in .env") + doctor.add_argument("--redis-url", help="check this Redis instead of the one in .env") + + restart = stack_command("restart", "restart", "restart the services, changing nothing else") + restart.add_argument("services", nargs="*", help="services to restart, e.g. api worker") + env = stack_command("env", "env", "show, get or set the stack's settings") env_actions = env.add_subparsers(dest="env_action", metavar="") get = env_actions.add_parser("get", help="print one setting") get.add_argument("key") - set_ = env_actions.add_parser("set", help="set settings (KEY=VALUE ...); run `docsgpt up` to apply") + set_ = env_actions.add_parser("set", help="set settings (KEY=VALUE ...)") set_.add_argument("pairs", nargs="+", metavar="KEY=VALUE") + set_.add_argument("--no-restart", dest="restart", action="store_false", + help="do not restart a running native install afterwards") + + dev = commands.add_parser("dev", help="run this checkout's API, worker and UI with reload") + dev.add_argument("--host", default=DEFAULT_HOST, help="interface for the API (default: localhost)") + dev.add_argument("--port", type=int, default=DEFAULT_PORT, help="port for the API (default: 7091)") + dev.add_argument("--ui", action="store_true", help="also run the frontend dev server") + dev.add_argument("--mock-llm", action="store_true", + help="run the mock LLM and point DocsGPT at it, so no API key is needed") + dev.add_argument("--no-worker", dest="worker", action="store_false", help="do not run the Celery worker") + dev.add_argument("--no-reload", dest="reload", action="store_false", + help="do not restart the API and worker when a file changes") + dev.add_argument("-l", "--loglevel", default="INFO", help="worker log level (default: INFO)") + dev.set_defaults(func=_deploy("dev"), deploy=True) def build_parser() -> argparse.ArgumentParser: diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 0d95655f..8b1503a6 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -12,6 +12,7 @@ import socket import subprocess import sys import tempfile +import time import webbrowser from collections.abc import Callable, Mapping from dataclasses import dataclass @@ -623,12 +624,36 @@ def logs(args, context: Optional[Context] = None) -> int: else: print("(nothing logged yet)") if args.follow: - print("Following is not supported in native mode; use `tail -f` on the files above.", file=sys.stderr) + return _follow(logs_dir, wanted) return 0 options = (["--follow"] if args.follow else []) + (["--tail", str(args.tail)] if args.tail else []) return context.docker.compose(directory, "logs", *options, *args.services, check=False).returncode +def _follow(logs_dir: Path, services: list) -> int: + """Print new lines from each service's log until the terminal interrupts, prefixed by service.""" + handles: dict = {} + try: + while True: + for service in services: + if service not in handles: + path = logs_dir / f"{service}.log" + if not path.is_file(): + continue + handle = path.open("r", encoding="utf-8", errors="replace") + handle.seek(0, os.SEEK_END) + handles[service] = handle + for line in handles[service].readlines(): + print(f"{service:<6} | {line.rstrip()}") + sys.stdout.flush() + time.sleep(0.3) + except KeyboardInterrupt: + return 0 + finally: + for handle in handles.values(): + handle.close() + + def token(args, context: Optional[Context] = None) -> int: """Print the access token of a ``simple_jwt`` install.""" directory = stack.stack_dir(args.dir) @@ -681,10 +706,215 @@ def env(args, context: Optional[Context] = None) -> int: envfile.update(env_path, updates) except ValueError as exc: raise DeployError(str(exc)) from exc - print(f"Saved to {env_path}. Run `docsgpt up` to apply.") + print(f"Saved to {env_path}.") + directory = stack.stack_dir(args.dir) + if _mode(directory) == "native" and getattr(args, "restart", True): + context = context or Context.default(args) + services = context.service_manager() + names = _service_names(directory) + if any(services.is_running(name) for name in names): + for name in reversed(names): + services.stop(name) + for name in names: + services.start(name) + print("Restarted the services, so the change is live.") + return 0 + print("Run `docsgpt up` to apply.") return 0 +def dev(args, context: Optional[Context] = None) -> int: + """Run this checkout's API, worker and UI as children of this terminal.""" + from docsgpt.core import paths + from docsgpt.deploy import dev as dev_module + + checkout = paths.checkout_root() + if checkout is None: + raise DeployError( + "`docsgpt dev` runs the code in a source checkout, and this is an installed package. " + "Clone the repository and run it from there, or use `docsgpt up --native` to run this copy." + ) + if not _port_is_free(args.port): + raise DeployError( + f"port {args.port} is already in use, so the API cannot bind it. Stop what is on it " + f"(a previous `docsgpt dev`, or `docsgpt down` for an install), or pass --port." + ) + children = dev_module.plan(args, checkout) + print(f"DocsGPT from {checkout}") + for child in children: + print(f" {child.name:<6} {' '.join(child.command)}") + print(f"\nAPI http://{args.host}:{args.port}") + if getattr(args, "ui", False): + print(f"UI http://localhost:{dev_module.UI_PORT}") + print("Ctrl-C stops everything.\n") + return dev_module.run(children) + + +@dataclass +class Check: + """One line of ``docsgpt doctor``: what was looked at and what came back.""" + + name: str + level: str + detail: str + + +MARKS = {"ok": "ok ", "warn": "warn", "fail": "FAIL"} + + +def _migration_head() -> Optional[str]: + """The newest revision shipped with this package, or None when alembic cannot say.""" + try: + from alembic.config import Config + from alembic.script import ScriptDirectory + except ImportError: + return None + ini = Path(__file__).resolve().parents[1] / "alembic.ini" + if not ini.is_file(): + return None + config = Config(str(ini)) + config.set_main_option("script_location", str(ini.parent / "alembic")) + try: + return ScriptDirectory.from_config(config).get_current_head() + except Exception: # noqa: BLE001 - a broken script directory is a doctor finding, not a crash + return None + + +def _check_postgres(uri: Optional[str]) -> Check: + """Connect, and say whether the schema is the one this version expects.""" + if not uri: + return Check("postgres", "fail", "POSTGRES_URI is not set") + try: + import psycopg + except ImportError: + return Check("postgres", "fail", "the psycopg driver is not installed") + try: + with psycopg.connect(uri, connect_timeout=5) as connection, connection.cursor() as cursor: + cursor.execute("select current_setting('server_version')") + version = cursor.fetchone()[0] + cursor.execute("select to_regclass('public.alembic_version')") + applied = cursor.fetchone()[0] is not None + current = None + if applied: + cursor.execute("select version_num from alembic_version") + row = cursor.fetchone() + current = row[0] if row else None + except (psycopg.Error, OSError, ValueError) as exc: + return Check("postgres", "fail", f"cannot connect: {str(exc).strip()}") + head = _migration_head() + if not current: + return Check("postgres", "fail", f"PostgreSQL {version}, no schema yet; run `docsgpt migrate`") + if head and current != head: + return Check("postgres", "fail", f"PostgreSQL {version} at {current}, this version wants {head}; " + "run `docsgpt migrate`") + return Check("postgres", "ok", f"PostgreSQL {version}, schema at {current}") + + +def _check_redis(urls: Mapping[str, str]) -> Check: + """Ping every Redis the settings name; they are usually one server, three databases.""" + if not urls: + return Check("redis", "fail", "no Redis is configured (CELERY_BROKER_URL)") + try: + import redis + except ImportError: + return Check("redis", "fail", "the redis client is not installed") + for label, url in sorted(urls.items()): + try: + redis.Redis.from_url(url, socket_connect_timeout=3).ping() + except Exception as exc: # noqa: BLE001 - every client error here is the same finding + return Check("redis", "fail", f"{label} ({url}) does not answer: {str(exc).strip()}") + return Check("redis", "ok", f"answering on {len(urls)} database(s)") + + +def _check_provider(env: Mapping[str, str]) -> Check: + """Whether a model provider is set up well enough to answer a question.""" + provider = env.get("LLM_PROVIDER") or "docsgpt" + if provider == "docsgpt": + return Check("provider", "ok", "the DocsGPT public API (no key needed)") + if not (env.get("API_KEY") or env.get("OPENAI_API_KEY")): + return Check("provider", "fail", f"{provider} is configured but no API_KEY is set") + return Check("provider", "ok", f"{provider}{' at ' + env['OPENAI_BASE_URL'] if env.get('OPENAI_BASE_URL') else ''}") + + +def doctor(args, context: Optional[Context] = None) -> int: + """Check what DocsGPT needs on this machine, and say what is missing.""" + from docsgpt.core import paths + + if args.dir: + env_path = stack.stack_dir(args.dir) / ".env" + else: + try: + env_path = paths.env_file() + except FileNotFoundError as exc: + raise DeployError(str(exc)) from exc + env = envfile.read(env_path) + checks = [ + Check("settings", "ok" if env_path.is_file() else "warn", + f"{env_path}" if env_path.is_file() else f"{env_path} does not exist yet; defaults are in use"), + _check_postgres(args.postgres_uri or env.get("POSTGRES_URI")), + _check_redis({ + key: value for key, value in ( + ("broker", args.redis_url or env.get("CELERY_BROKER_URL")), + ("results", env.get("CELERY_RESULT_BACKEND")), + ("cache", env.get("CACHE_REDIS_URL")), + ) if value + }), + _check_provider(env), + ] + + port = int(env.get("DOCSGPT_PORT") or stack.DEFAULT_PORT) + if _port_is_free(port): + checks.append(Check("port", "ok", f"{port} is free")) + else: + checks.append(Check("port", "warn", f"{port} is in use, which is expected if DocsGPT is running")) + + directory = stack.stack_dir(args.dir) + if _mode(directory) == "native": + services = context.service_manager() if context else native.services_for_platform() + names = _service_names(directory) + running = [name for name in names if services.is_running(name)] + level = "ok" if len(running) == len(names) else "warn" + checks.append(Check("services", level, f"{len(running)} of {len(names)} running ({', '.join(names)})")) + + for check in checks: + print(f"[{MARKS[check.level]}] {check.name:<9} {check.detail}") + failed = [check for check in checks if check.level == "fail"] + if failed: + print(f"\n{len(failed)} problem(s) to fix before DocsGPT will work.", file=sys.stderr) + return 1 if failed else 0 + + +def _chosen_services(names: tuple[str, str], wanted: list) -> list: + """The services the user asked for, given either short names (api) or full ones.""" + if not wanted: + return list(names) + chosen = [] + for ask in wanted: + match = [name for name in names if name == ask or name.removeprefix("docsgpt-").startswith(ask)] + if not match: + raise DeployError(f"{ask!r} is not a service of this install; it has {', '.join(names)}.") + chosen.extend(match) + return chosen + + +def restart(args, context: Optional[Context] = None) -> int: + """Restart the services, changing nothing else.""" + context = context or Context.default(args) + directory = stack.stack_dir(args.dir) + if _installed(directory) is None: + return 1 + if _mode(directory) == "native": + services = context.service_manager() + chosen = _chosen_services(_service_names(directory), list(args.services)) + for name in reversed(chosen): + services.stop(name) + for name in chosen: + services.start(name) + print(f"Restarted {', '.join(chosen)}.") + return 0 + return context.docker.compose(directory, "restart", *args.services, check=False).returncode + + def _stack_image(env: Mapping[str, str]) -> str: """The image this install runs; the volume tars go through it, so nothing extra is pulled.""" tag = env.get("DOCSGPT_IMAGE_TAG") or "latest" diff --git a/docsgpt/deploy/dev.py b/docsgpt/deploy/dev.py new file mode 100644 index 00000000..a49dcf07 --- /dev/null +++ b/docsgpt/deploy/dev.py @@ -0,0 +1,214 @@ +"""``docsgpt dev``: this checkout's API, worker and UI as children of one terminal. + +``docsgpt up --native`` installs services meant to outlive the shell. Development wants the +opposite: processes rooted in the checkout, restarting when a file is saved, logging into one +terminal, and gone when Ctrl-C lands. This module decides which processes to run and supervises +them; nothing here imports the app itself. +""" + +from __future__ import annotations + +import os +import shlex +import shutil +import signal +import subprocess +import sys +import threading +import time +from dataclasses import dataclass, field +from pathlib import Path +from typing import Callable, Optional, TextIO + +from docsgpt.deploy.docker import DeployError + +MOCK_LLM_PORT = 8090 +UI_PORT = 5173 +STOP_GRACE = 10.0 + +# One colour per child so a glance at the terminal says who is talking. +COLOURS = {"api": "\033[36m", "worker": "\033[35m", "ui": "\033[32m", "llm": "\033[33m"} +RESET = "\033[0m" +WIDTH = 6 + + +@dataclass +class Child: + """One process ``docsgpt dev`` runs.""" + + name: str + command: list[str] + cwd: Path + env: dict[str, str] = field(default_factory=dict) + + +def watchfiles_available() -> bool: + """Whether the worker can be restarted on save; it arrives with uvicorn's standard extras.""" + try: + import watchfiles # noqa: F401 + except ImportError: + return False + return True + + +def _reloading_command(command: list[str], watched: Path) -> list[str]: + """``command`` under watchfiles, restarted when a Python file under ``watched`` changes.""" + return [sys.executable, "-m", "watchfiles", "--filter", "python", shlex.join(command), str(watched)] + + +def plan( + args, + checkout: Path, + *, + watching: Optional[bool] = None, + launcher: Optional[list[str]] = None, +) -> list[Child]: + """The children to run, in the order they should start.""" + from docsgpt.deploy import stack + + launcher = launcher or [sys.executable, "-m", "docsgpt"] + watching = watchfiles_available() if watching is None else watching + package = checkout / "docsgpt" + environment = {"DOCSGPT_HOME": str(checkout)} + children: list[Child] = [] + + if getattr(args, "mock_llm", False): + script = checkout / "scripts" / "mock_llm.py" + if not script.is_file(): + raise DeployError(f"{script} is missing, so there is no mock LLM to run.") + children.append( + Child( + name="llm", + command=[sys.executable, str(script), "--port", str(MOCK_LLM_PORT)], + cwd=checkout, + env=dict(environment), + ) + ) + # The children read these from the environment, so the checkout's .env is left alone. + chosen = stack.provider_settings( + "openai-compatible", model="mock", base_url=f"http://127.0.0.1:{MOCK_LLM_PORT}/v1" + ) + environment.update({key: value for key, value in chosen.items() if value is not None}) + + api = [*launcher, "api", "--host", args.host, "--port", str(args.port)] + if getattr(args, "reload", True): + api.append("--reload") + children.append(Child(name="api", command=api, cwd=checkout, env=dict(environment))) + + if getattr(args, "worker", True): + worker = [*launcher, "worker", "-l", getattr(args, "loglevel", "INFO")] + if getattr(args, "reload", True) and watching: + worker = _reloading_command(worker, package) + children.append(Child(name="worker", command=worker, cwd=checkout, env=dict(environment))) + + if getattr(args, "ui", False): + frontend = checkout / "frontend" + if not (frontend / "node_modules").is_dir(): + raise DeployError( + f"the frontend has no node_modules yet. Run `npm install --include=dev` in {frontend} " + "and try again, or leave --ui off." + ) + if not shutil.which("npm"): + raise DeployError("npm is not on PATH, so the frontend dev server cannot start.") + children.append(Child(name="ui", command=["npm", "run", "dev"], cwd=frontend, env=dict(environment))) + + return children + + +def _line(name: str, text: str, colour: bool) -> str: + """One output line, prefixed with the child that wrote it.""" + label = name.ljust(WIDTH) + if colour: + return f"{COLOURS.get(name, '')}{label}{RESET} | {text}" + return f"{label} | {text}" + + +def _pump(child: Child, process, out: TextIO, lock: threading.Lock, colour: bool) -> None: + """Copy one child's output to ``out``, a line at a time, prefixed.""" + stream = process.stdout + if stream is None: + return + for text in stream: + with lock: + out.write(_line(child.name, text.rstrip("\n"), colour)) + out.write("\n") + out.flush() + + +def _signal(process, number: int) -> None: + """Signal a child and, on POSIX, everything it started.""" + try: + if os.name == "nt": + process.terminate() + return + os.killpg(os.getpgid(process.pid), number) + except (ProcessLookupError, PermissionError, OSError): + pass + + +def _stop(running: list[tuple[Child, object]], grace: float, sleep: Callable[[float], None]) -> None: + """Interrupt the children, then insist if they are still there. + + A second Ctrl-C lands while this is waiting. It means "stop waiting", not "give up": the wait + ends and the children are killed, rather than the interrupt escaping and leaving them running. + """ + for _, process in running: + if process.poll() is None: + _signal(process, signal.SIGINT) + deadline = time.monotonic() + grace + while time.monotonic() < deadline and any(process.poll() is None for _, process in running): + try: + sleep(0.1) + except KeyboardInterrupt: + break + for _, process in running: + if process.poll() is None: + _signal(process, signal.SIGKILL) + + +def run( + children: list[Child], + *, + out: TextIO = sys.stdout, + spawn: Callable[..., object] = subprocess.Popen, + sleep: Callable[[float], None] = time.sleep, + grace: float = STOP_GRACE, + colour: Optional[bool] = None, +) -> int: + """Start the children and keep them running until one exits or the terminal interrupts.""" + colour = out.isatty() if colour is None else colour + lock = threading.Lock() + running: list[tuple[Child, object]] = [] + try: + for child in children: + process = spawn( + child.command, + cwd=str(child.cwd), + env={**os.environ, **child.env}, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + bufsize=1, + # Its own session, so Ctrl-C reaches this process and the children are stopped in order. + start_new_session=os.name != "nt", + ) + running.append((child, process)) + threading.Thread(target=_pump, args=(child, process, out, lock, colour), daemon=True).start() + + while True: + for child, process in running: + code = process.poll() + if code is not None: + with lock: + out.write(_line(child.name, f"exited with {code}", colour)) + out.write("\n") + out.flush() + return code or 1 + sleep(0.2) + except KeyboardInterrupt: + with lock: + out.write("\nStopping ...\n") + out.flush() + return 0 + finally: + _stop(running, grace, sleep) diff --git a/tests/deploy/test_dev.py b/tests/deploy/test_dev.py new file mode 100644 index 00000000..a8344954 --- /dev/null +++ b/tests/deploy/test_dev.py @@ -0,0 +1,180 @@ +"""`docsgpt dev`: the checkout's processes as children of one terminal.""" + +import argparse +import io +import signal +import sys +from pathlib import Path + +import pytest + +from docsgpt.deploy import dev +from docsgpt.deploy.docker import DeployError + + +def _args(**overrides): + values = {"host": "127.0.0.1", "port": 7091, "ui": False, "mock_llm": False, + "worker": True, "reload": True, "loglevel": "INFO"} + values.update(overrides) + return argparse.Namespace(**values) + + +def _named(children): + return [child.name for child in children] + + +class FakeProcess: + """A child that produces the given lines and is done.""" + + def __init__(self, lines=(), code=None): + self.stdout = iter(list(lines)) + self.code = code + self.pid = -1 + + def poll(self): + return self.code + + def terminate(self): + self.code = self.code if self.code is not None else -15 + + +class TestPlan: + def test_the_api_and_worker_run_from_the_checkout(self, tmp_path): + children = dev.plan(_args(), tmp_path, watching=False) + assert _named(children) == ["api", "worker"] + api, worker = children + assert api.command[-6:-1] == ["api", "--host", "127.0.0.1", "--port", "7091"] + assert api.command[-1] == "--reload" + assert worker.command[-2:] == ["-l", "INFO"] + for child in children: + assert child.cwd == tmp_path + assert child.env["DOCSGPT_HOME"] == str(tmp_path), "the checkout is the data home, not ~/.docsgpt" + + def test_the_worker_restarts_on_save_when_watchfiles_is_there(self, tmp_path): + """Celery has no reloader of its own, so it is wrapped in one.""" + worker = dev.plan(_args(), tmp_path, watching=True)[1] + assert "watchfiles" in worker.command + assert str(tmp_path / "docsgpt") in worker.command, "it watches the package, not the whole checkout" + assert "docsgpt worker" in " ".join(worker.command) + + def test_without_watchfiles_the_worker_still_runs(self, tmp_path): + worker = dev.plan(_args(), tmp_path, watching=False)[1] + assert "watchfiles" not in " ".join(worker.command) + + def test_no_reload_leaves_both_alone(self, tmp_path): + children = dev.plan(_args(reload=False), tmp_path, watching=True) + assert "--reload" not in children[0].command + assert "watchfiles" not in " ".join(children[1].command) + + def test_no_worker(self, tmp_path): + assert _named(dev.plan(_args(worker=False), tmp_path, watching=False)) == ["api"] + + def test_the_mock_llm_starts_first_and_the_others_are_pointed_at_it(self, tmp_path): + """A dev loop that needs no API key: the mock has to be up before the API asks it anything.""" + (tmp_path / "scripts").mkdir() + (tmp_path / "scripts" / "mock_llm.py").write_text("", encoding="utf-8") + children = dev.plan(_args(mock_llm=True), tmp_path, watching=False) + assert _named(children) == ["llm", "api", "worker"] + api = children[1] + assert api.env["LLM_PROVIDER"] == "openai" + assert api.env["OPENAI_BASE_URL"] == f"http://127.0.0.1:{dev.MOCK_LLM_PORT}/v1" + assert api.env["API_KEY"], "the client wants some key, even a placeholder" + assert "OPENAI_BASE_URL" not in children[0].env, "the mock itself does not need pointing at itself" + + def test_a_missing_mock_llm_script_is_reported(self, tmp_path): + with pytest.raises(DeployError, match="mock LLM"): + dev.plan(_args(mock_llm=True), tmp_path, watching=False) + + def test_the_ui_needs_its_dependencies(self, tmp_path): + (tmp_path / "frontend").mkdir() + with pytest.raises(DeployError, match="node_modules"): + dev.plan(_args(ui=True), tmp_path, watching=False) + + def test_the_ui_needs_npm_on_path(self, tmp_path, monkeypatch): + (tmp_path / "frontend" / "node_modules").mkdir(parents=True) + monkeypatch.setattr(dev.shutil, "which", lambda name: None) + with pytest.raises(DeployError, match="npm"): + dev.plan(_args(ui=True), tmp_path, watching=False) + + def test_the_ui_runs_in_the_frontend_directory(self, tmp_path, monkeypatch): + (tmp_path / "frontend" / "node_modules").mkdir(parents=True) + monkeypatch.setattr(dev.shutil, "which", lambda name: "/usr/local/bin/npm") + children = dev.plan(_args(ui=True), tmp_path, watching=False) + ui = children[-1] + assert ui.name == "ui" + assert ui.command == ["npm", "run", "dev"] + assert ui.cwd == tmp_path / "frontend" + + +class TestRun: + def _spawn(self, processes): + made = iter(processes) + + def spawn(command, **kwargs): + return next(made) + + return spawn + + def test_output_is_prefixed_with_the_child_that_wrote_it(self, tmp_path): + out = io.StringIO() + children = [dev.Child(name="api", command=["true"], cwd=tmp_path)] + code = dev.run(children, out=out, spawn=self._spawn([FakeProcess(["hello\n"], code=0)]), + sleep=lambda _: None, colour=False) + assert "api | hello" in out.getvalue() + assert code == 1, "a child that ends by itself ends the session, however it exited" + + def test_a_failing_child_returns_its_code(self, tmp_path): + out = io.StringIO() + children = [dev.Child(name="api", command=["false"], cwd=tmp_path)] + code = dev.run(children, out=out, spawn=self._spawn([FakeProcess(code=2)]), + sleep=lambda _: None, colour=False) + assert code == 2 + assert "exited with 2" in out.getvalue() + + def test_an_interrupt_stops_quietly(self, tmp_path): + out = io.StringIO() + + def sleep(_): + raise KeyboardInterrupt + + children = [dev.Child(name="api", command=["sleep"], cwd=tmp_path)] + code = dev.run(children, out=out, spawn=self._spawn([FakeProcess()]), sleep=sleep, colour=False) + assert code == 0 + assert "Stopping" in out.getvalue() + + def test_a_second_interrupt_during_shutdown_still_kills(self, tmp_path, monkeypatch): + """Ctrl-C twice is what you press when it did not die; it must not leave children behind.""" + sent = [] + monkeypatch.setattr(dev, "_signal", lambda process, number: sent.append(number)) + + def sleep(_): + raise KeyboardInterrupt + + children = [dev.Child(name="api", command=["sleep"], cwd=tmp_path)] + code = dev.run(children, out=io.StringIO(), spawn=self._spawn([FakeProcess()]), + sleep=sleep, colour=False, grace=5) + assert code == 0 + assert signal.SIGINT in sent + assert signal.SIGKILL in sent, "the second interrupt escalates instead of escaping" + + def test_children_get_the_checkout_environment(self, tmp_path, monkeypatch): + seen = {} + + def spawn(command, **kwargs): + seen.update(kwargs) + return FakeProcess(code=0) + + monkeypatch.setenv("SOMETHING_ELSE", "kept") + children = [dev.Child(name="api", command=["x"], cwd=tmp_path, env={"DOCSGPT_HOME": str(tmp_path)})] + dev.run(children, out=io.StringIO(), spawn=spawn, sleep=lambda _: None, colour=False) + assert seen["env"]["DOCSGPT_HOME"] == str(tmp_path) + assert seen["env"]["SOMETHING_ELSE"] == "kept", "the shell's environment is kept, not replaced" + assert seen["cwd"] == str(tmp_path) + assert seen["start_new_session"] is (sys.platform != "win32") + + +class TestReloadingCommand: + def test_an_interpreter_path_with_spaces_survives(self): + command = dev._reloading_command(["/opt/my venv/bin/python", "-m", "docsgpt", "worker"], Path("/srv/pkg")) + assert "'/opt/my venv/bin/python' -m docsgpt worker" in command + assert command[-1] == "/srv/pkg" diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py new file mode 100644 index 00000000..1309d512 --- /dev/null +++ b/tests/deploy/test_doctor.py @@ -0,0 +1,167 @@ +"""`docsgpt doctor`, `restart`, following native logs, and settings that apply themselves.""" + +import pytest + +from docsgpt.deploy import commands, envfile +from docsgpt.deploy.docker import DeployError + +from .test_commands import FakeDocker, _context, _run +from .test_native import FakeServices, _names, _native_context + + +def _installed_native(tmp_path, services): + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] + assert _run(argv, _native_context(services)) == 0 + + +class TestRestart: + def test_it_stops_and_starts_both_without_touching_settings(self, tmp_path): + services = FakeServices() + _installed_native(tmp_path, services) + before = (tmp_path / ".env").read_text(encoding="utf-8") + services.started.clear() + services.stopped.clear() + + assert _run(["restart", "--dir", str(tmp_path)], _native_context(services)) == 0 + assert services.stopped == list(reversed(_names(tmp_path))), "the worker goes down first" + assert services.started == list(_names(tmp_path)) + assert (tmp_path / ".env").read_text(encoding="utf-8") == before + + def test_one_service_by_its_short_name(self, tmp_path): + services = FakeServices() + _installed_native(tmp_path, services) + services.started.clear() + assert _run(["restart", "api", "--dir", str(tmp_path)], _native_context(services)) == 0 + assert services.started == [_names(tmp_path)[0]] + + def test_a_name_this_install_does_not_have(self, tmp_path): + services = FakeServices() + _installed_native(tmp_path, services) + with pytest.raises(DeployError, match="not a service"): + _run(["restart", "frontend", "--dir", str(tmp_path)], _native_context(services)) + + def test_a_docker_install_restarts_its_containers(self, tmp_path): + assert _run(["up", "--yes", "--dir", str(tmp_path)], _context()) == 0 + docker = FakeDocker() + assert _run(["restart", "--dir", str(tmp_path)], _context(docker)) == 0 + assert ["restart"] in [args for _, args in docker.calls] + + +class TestEnvApplies: + def test_a_running_native_install_restarts_itself(self, tmp_path, capsys): + """The old advice was to run `docsgpt up` again, which reruns migrations to change one value.""" + services = FakeServices() + _installed_native(tmp_path, services) + services.started.clear() + + argv = ["env", "--dir", str(tmp_path), "set", "LLM_NAME=gpt-4o"] + assert _run(argv, _native_context(services)) == 0 + assert envfile.read(tmp_path / ".env")["LLM_NAME"] == "gpt-4o" + assert services.started == list(_names(tmp_path)) + assert "Restarted" in capsys.readouterr().out + + def test_no_restart_leaves_the_services_alone(self, tmp_path, capsys): + services = FakeServices() + _installed_native(tmp_path, services) + services.started.clear() + + argv = ["env", "--dir", str(tmp_path), "set", "LLM_NAME=gpt-4o", "--no-restart"] + assert _run(argv, _native_context(services)) == 0 + assert services.started == [] + assert "docsgpt up" in capsys.readouterr().out + + def test_a_stopped_install_is_not_started_by_a_settings_change(self, tmp_path): + services = FakeServices() + _installed_native(tmp_path, services) + for name in _names(tmp_path): + services.stop(name) + services.started.clear() + + argv = ["env", "--dir", str(tmp_path), "set", "LLM_NAME=gpt-4o"] + assert _run(argv, _native_context(services)) == 0 + assert services.started == [], "changing a setting does not start a stopped install" + + +class TestFollowLogs: + def test_it_prints_lines_written_after_it_started(self, tmp_path, capsys, monkeypatch): + logs = tmp_path / "logs" + logs.mkdir() + (logs / "api.log").write_text("old line\n", encoding="utf-8") + + rounds = {"n": 0} + + def sleep(_): + rounds["n"] += 1 + if rounds["n"] == 1: + (logs / "api.log").open("a", encoding="utf-8").write("new line\n") + return + raise KeyboardInterrupt + + monkeypatch.setattr(commands.time, "sleep", sleep) + assert commands._follow(logs, ["api"]) == 0 + printed = capsys.readouterr().out + assert "api | new line" in printed + assert "old line" not in printed, "it starts at the end, like tail -f" + + +class TestChecks: + def test_the_public_api_needs_no_key(self): + check = commands._check_provider({"LLM_PROVIDER": "docsgpt"}) + assert check.level == "ok" + + def test_a_provider_without_a_key_is_a_problem(self): + check = commands._check_provider({"LLM_PROVIDER": "openai"}) + assert check.level == "fail" + assert "API_KEY" in check.detail + + def test_a_provider_with_a_key_and_a_base_url(self): + check = commands._check_provider( + {"LLM_PROVIDER": "openai", "API_KEY": "x", "OPENAI_BASE_URL": "http://localhost:8090/v1"} + ) + assert check.level == "ok" + assert "8090" in check.detail + + def test_services_are_named_for_the_install(self, tmp_path): + names = _names(tmp_path) + assert commands._chosen_services(names, []) == list(names) + assert commands._chosen_services(names, ["worker"]) == [names[1]] + assert commands._chosen_services(names, [names[0]]) == [names[0]] + + +class TestDoctor: + def _only(self, monkeypatch, postgres, redis): + monkeypatch.setattr(commands, "_check_postgres", lambda uri: postgres) + monkeypatch.setattr(commands, "_check_redis", lambda urls: redis) + + def test_it_reports_every_check_and_succeeds_when_they_pass(self, tmp_path, capsys, monkeypatch): + self._only( + monkeypatch, + commands.Check("postgres", "ok", "PostgreSQL 16.2, schema at 0031"), + commands.Check("redis", "ok", "answering on 3 database(s)"), + ) + (tmp_path / ".env").write_text("LLM_PROVIDER=docsgpt\n", encoding="utf-8") + assert _run(["doctor", "--dir", str(tmp_path)], _context()) == 0 + out = capsys.readouterr().out + assert "postgres" in out and "redis" in out and "provider" in out + + def test_a_failing_check_makes_it_exit_one(self, tmp_path, capsys, monkeypatch): + self._only( + monkeypatch, + commands.Check("postgres", "fail", "cannot connect: refused"), + commands.Check("redis", "ok", "answering"), + ) + (tmp_path / ".env").write_text("LLM_PROVIDER=docsgpt\n", encoding="utf-8") + assert _run(["doctor", "--dir", str(tmp_path)], _context()) == 1 + captured = capsys.readouterr() + assert "FAIL" in captured.out + assert "1 problem" in captured.err + + def test_it_says_which_settings_file_it_read(self, tmp_path, capsys, monkeypatch): + self._only( + monkeypatch, + commands.Check("postgres", "ok", "fine"), + commands.Check("redis", "ok", "fine"), + ) + (tmp_path / ".env").write_text("LLM_PROVIDER=docsgpt\n", encoding="utf-8") + _run(["doctor", "--dir", str(tmp_path)], _context()) + assert str(tmp_path / ".env") in capsys.readouterr().out diff --git a/tests/test_cli.py b/tests/test_cli.py index dcad465e..bdb2977d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -10,6 +10,7 @@ import click import pytest from docsgpt import cli +from docsgpt.core.paths import package_dir from docsgpt.version import __version__ @@ -126,7 +127,28 @@ class TestApi: uvicorn = types.SimpleNamespace(run=MagicMock()) monkeypatch.setitem(sys.modules, "uvicorn", uvicorn) assert cli.main(["api", "--reload", "--host", "127.0.0.1"]) == 0 - uvicorn.run.assert_called_once_with("docsgpt.asgi:asgi_app", host="127.0.0.1", port=7091, reload=True) + uvicorn.run.assert_called_once_with( + "docsgpt.asgi:asgi_app", host="127.0.0.1", port=7091, reload=True, + reload_dirs=[str(package_dir())], + ) + + def test_reload_watches_the_package_not_the_working_directory(self, monkeypatch, tmp_path): + """A checkout also holds .venv, node_modules and the indexes and inputs the app writes to, + so watching the working directory restarts the server mid-ingest.""" + uvicorn = types.SimpleNamespace(run=MagicMock()) + monkeypatch.setitem(sys.modules, "uvicorn", uvicorn) + monkeypatch.chdir(tmp_path) + assert cli.main(["api", "--reload"]) == 0 + watched = uvicorn.run.call_args.kwargs["reload_dirs"] + assert watched == [str(package_dir())] + assert str(tmp_path) not in watched + + def test_without_reload_nothing_is_watched(self, monkeypatch): + uvicorn = types.SimpleNamespace(run=MagicMock()) + monkeypatch.setitem(sys.modules, "uvicorn", uvicorn) + monkeypatch.setattr(sys, "platform", "win32") + assert cli.main(["api"]) == 0 + assert uvicorn.run.call_args.kwargs["reload_dirs"] is None class TestWorker: From 731baa7d31c145698c8ffd55876e21c02c9cb3d4 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 11:43:06 +0100 Subject: [PATCH 02/14] test: cover what doctor actually tells you The checks were mocked wholesale, so the branching that produces each diagnosis had never run: a database with no schema yet, one behind this version, one that refuses the connection, and which of the three Redis URLs failed. Each of those is the sentence a developer reads when something is wrong, so each is pinned. _migration_head is tested against the packaged alembic.ini itself: it needs no database, and it is the path resolution that breaks silently when files move. --- tests/deploy/test_doctor.py | 118 ++++++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 1309d512..b92104ce 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -128,6 +128,124 @@ class TestChecks: assert commands._chosen_services(names, [names[0]]) == [names[0]] +class FakeCursor: + """Answers doctor's three queries in order: server version, whether the table is there, the revision.""" + + def __init__(self, version, table, revision): + self.answers = [(version,), (table,), (revision,) if revision else None] + self.given = 0 + + def execute(self, statement): + self.statement = statement + + def fetchone(self): + answer = self.answers[self.given] + self.given += 1 + return answer + + def __enter__(self): + return self + + def __exit__(self, *exception): + return False + + +class FakeConnection: + def __init__(self, cursor): + self._cursor = cursor + + def cursor(self): + return self._cursor + + def __enter__(self): + return self + + def __exit__(self, *exception): + return False + + +def _postgres_answering(monkeypatch, version="16.2", table="alembic_version", revision="0031_x", head="0031_x"): + import psycopg + + monkeypatch.setattr(psycopg, "connect", lambda *a, **k: FakeConnection(FakeCursor(version, table, revision))) + monkeypatch.setattr(commands, "_migration_head", lambda: head) + + +class TestPostgresCheck: + def test_at_head(self, monkeypatch): + _postgres_answering(monkeypatch) + check = commands._check_postgres("postgresql://localhost/d") + assert check.level == "ok" + assert "16.2" in check.detail and "0031_x" in check.detail + + def test_a_database_with_no_schema_yet(self, monkeypatch): + """The commonest first-run state: the database exists, nothing has been migrated into it.""" + _postgres_answering(monkeypatch, table=None, revision=None) + check = commands._check_postgres("postgresql://localhost/d") + assert check.level == "fail" + assert "docsgpt migrate" in check.detail + + def test_a_schema_behind_this_version(self, monkeypatch): + _postgres_answering(monkeypatch, revision="0029_old", head="0031_x") + check = commands._check_postgres("postgresql://localhost/d") + assert check.level == "fail" + assert "0029_old" in check.detail and "0031_x" in check.detail + assert "docsgpt migrate" in check.detail + + def test_a_database_that_does_not_answer(self, monkeypatch): + import psycopg + + def refuse(*args, **kwargs): + raise psycopg.OperationalError("connection refused") + + monkeypatch.setattr(psycopg, "connect", refuse) + check = commands._check_postgres("postgresql://localhost/d") + assert check.level == "fail" + assert "connection refused" in check.detail + + def test_without_a_uri_at_all(self): + check = commands._check_postgres(None) + assert check.level == "fail" + assert "POSTGRES_URI" in check.detail + + +class TestRedisCheck: + def test_every_database_answering(self, monkeypatch): + import redis + + monkeypatch.setattr(redis.Redis, "from_url", classmethod(lambda cls, url, **k: type("R", (), {"ping": lambda self: True})())) + check = commands._check_redis({"broker": "redis://localhost:6379/0", "cache": "redis://localhost:6379/2"}) + assert check.level == "ok" + assert "2" in check.detail + + def test_the_one_that_does_not_answer_is_named(self, monkeypatch): + """Three URLs usually differ only by database number, so the message has to say which.""" + import redis + + def from_url(cls, url, **kwargs): + class Client: + def ping(self): + raise ConnectionError(f"no route to {url}") + + return Client() + + monkeypatch.setattr(redis.Redis, "from_url", classmethod(from_url)) + check = commands._check_redis({"cache": "redis://localhost:6379/2"}) + assert check.level == "fail" + assert "cache" in check.detail and "6379/2" in check.detail + + def test_without_any_redis_configured(self): + assert commands._check_redis({}).level == "fail" + + +class TestMigrationHead: + def test_it_finds_the_revision_this_package_ships(self): + """No database needed: this is the alembic.ini path resolution, which breaks silently.""" + head = commands._migration_head() + assert head, "the packaged alembic.ini should resolve to a revision" + assert head[0].isdigit(), head + + class TestDoctor: def _only(self, monkeypatch, postgres, redis): monkeypatch.setattr(commands, "_check_postgres", lambda uri: postgres) From bcf2707efa3dcbba5be6e2ab3e516f5c0fecf68f Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 11:43:53 +0100 Subject: [PATCH 03/14] fix: doctor reports a bad DOCSGPT_PORT instead of raising on it _port_number was written so a hand-edited .env could not reach int() raw, and then doctor did exactly that: a nonnumeric port ended the command with a traceback rather than the message, in the one command whose job is to explain a broken setup. --- docsgpt/deploy/commands.py | 4 +++- tests/deploy/test_doctor.py | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 8b1503a6..0811134f 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -862,7 +862,9 @@ def doctor(args, context: Optional[Context] = None) -> int: _check_provider(env), ] - port = int(env.get("DOCSGPT_PORT") or stack.DEFAULT_PORT) + # The one command that exists to explain a broken setup must not fall over on one. + port = _port_number(env["DOCSGPT_PORT"], f"DOCSGPT_PORT in {env_path}") if env.get("DOCSGPT_PORT") \ + else stack.DEFAULT_PORT if _port_is_free(port): checks.append(Check("port", "ok", f"{port} is free")) else: diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index b92104ce..3cc94dfd 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -274,6 +274,17 @@ class TestDoctor: assert "FAIL" in captured.out assert "1 problem" in captured.err + def test_a_hand_edited_port_is_reported_not_raised(self, tmp_path, monkeypatch): + """Doctor exists to explain a broken setup, so it must not traceback on one.""" + self._only( + monkeypatch, + commands.Check("postgres", "ok", "fine"), + commands.Check("redis", "ok", "fine"), + ) + (tmp_path / ".env").write_text("DOCSGPT_PORT=seven thousand\n", encoding="utf-8") + with pytest.raises(DeployError, match="not a port number"): + _run(["doctor", "--dir", str(tmp_path)], _context()) + def test_it_says_which_settings_file_it_read(self, tmp_path, capsys, monkeypatch): self._only( monkeypatch, From 9326c6edb1620369c2b9603016ddede6dffa152f Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 11:52:28 +0100 Subject: [PATCH 04/14] fix: keep credentials out of doctor's output, and read the revision from the schema it checked The Redis check put the whole URL in its failure message. A managed Redis URL carries user:password@host, so a failed ping printed the password to the terminal and into any log or issue the output was pasted into. It names scheme://host:port/db now, and falls back to naming no URL when the value cannot be parsed at all. The Postgres check asked to_regclass about public.alembic_version and then read version_num through search_path, so another schema could answer with a different revision, or the query could fail, and doctor would send you to run migrations against a database that is already fine. --- docsgpt/deploy/commands.py | 17 +++++++++++++++-- tests/deploy/test_doctor.py | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 0811134f..2da4929a 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -796,7 +796,8 @@ def _check_postgres(uri: Optional[str]) -> Check: applied = cursor.fetchone()[0] is not None current = None if applied: - cursor.execute("select version_num from alembic_version") + # public, like the to_regclass check above: search_path could resolve another one. + cursor.execute("select version_num from public.alembic_version") row = cursor.fetchone() current = row[0] if row else None except (psycopg.Error, OSError, ValueError) as exc: @@ -810,6 +811,18 @@ def _check_postgres(uri: Optional[str]) -> Check: return Check("postgres", "ok", f"PostgreSQL {version}, schema at {current}") +def _redis_endpoint(url: str) -> str: + """A Redis URL without its credentials: this ends up on a terminal, in CI logs and in issues.""" + try: + parts = urlsplit(url) + except ValueError: + return "the configured URL" + host = parts.hostname or "" + if parts.port: + host = f"{host}:{parts.port}" + return f"{parts.scheme}://{host}{parts.path}" if host else "the configured URL" + + def _check_redis(urls: Mapping[str, str]) -> Check: """Ping every Redis the settings name; they are usually one server, three databases.""" if not urls: @@ -822,7 +835,7 @@ def _check_redis(urls: Mapping[str, str]) -> Check: try: redis.Redis.from_url(url, socket_connect_timeout=3).ping() except Exception as exc: # noqa: BLE001 - every client error here is the same finding - return Check("redis", "fail", f"{label} ({url}) does not answer: {str(exc).strip()}") + return Check("redis", "fail", f"{label} ({_redis_endpoint(url)}) does not answer: {str(exc).strip()}") return Check("redis", "ok", f"answering on {len(urls)} database(s)") diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 3cc94dfd..9c73b77e 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -134,9 +134,10 @@ class FakeCursor: def __init__(self, version, table, revision): self.answers = [(version,), (table,), (revision,) if revision else None] self.given = 0 + self.statements = [] def execute(self, statement): - self.statement = statement + self.statements.append(statement) def fetchone(self): answer = self.answers[self.given] @@ -167,8 +168,10 @@ class FakeConnection: def _postgres_answering(monkeypatch, version="16.2", table="alembic_version", revision="0031_x", head="0031_x"): import psycopg - monkeypatch.setattr(psycopg, "connect", lambda *a, **k: FakeConnection(FakeCursor(version, table, revision))) + cursor = FakeCursor(version, table, revision) + monkeypatch.setattr(psycopg, "connect", lambda *a, **k: FakeConnection(cursor)) monkeypatch.setattr(commands, "_migration_head", lambda: head) + return cursor class TestPostgresCheck: @@ -178,6 +181,13 @@ class TestPostgresCheck: assert check.level == "ok" assert "16.2" in check.detail and "0031_x" in check.detail + def test_the_revision_comes_from_the_schema_that_was_checked(self, monkeypatch): + """to_regclass looks in public, so the second query must not resolve through search_path.""" + cursor = _postgres_answering(monkeypatch) + commands._check_postgres("postgresql://localhost/d") + revision_query = [statement for statement in cursor.statements if "version_num" in statement] + assert revision_query and all("public.alembic_version" in statement for statement in revision_query) + def test_a_database_with_no_schema_yet(self, monkeypatch): """The commonest first-run state: the database exists, nothing has been migrated into it.""" _postgres_answering(monkeypatch, table=None, revision=None) @@ -234,6 +244,27 @@ class TestRedisCheck: assert check.level == "fail" assert "cache" in check.detail and "6379/2" in check.detail + def test_a_password_in_the_url_is_never_printed(self, monkeypatch): + """doctor output goes into terminals, CI logs and pasted issue reports.""" + import redis + + def from_url(cls, url, **kwargs): + class Client: + def ping(self): + raise ConnectionError("connection refused") + + return Client() + + monkeypatch.setattr(redis.Redis, "from_url", classmethod(from_url)) + check = commands._check_redis({"broker": "rediss://default:sUpErSeCrEt@redis.example.com:6380/0"}) + assert check.level == "fail" + assert "sUpErSeCrEt" not in check.detail + assert "default" not in check.detail + assert "redis.example.com:6380/0" in check.detail, "the endpoint still has to be identifiable" + + def test_a_malformed_url_is_not_echoed_either(self): + assert commands._redis_endpoint("redis://[::1") == "the configured URL" + def test_without_any_redis_configured(self): assert commands._check_redis({}).level == "fail" From 18313c782371b9067635cb54d20e441fa5643330 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 12:01:28 +0100 Subject: [PATCH 05/14] fix: keep credentials out of the text doctor borrows from its clients Sanitising the URL in the message was not enough: the client's own error text went into the detail too, and both psycopg and redis-py quote the URL they were given. The reason is kept, the endpoint is kept, and the URL, username and password are taken out of it. The Redis test raised a generic error, so it asserted the password was absent without ever exercising the path that leaked. Both tests now raise what the clients actually raise. --- docsgpt/deploy/commands.py | 25 +++++++++++++++++++++---- tests/deploy/test_doctor.py | 21 +++++++++++++++++++-- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 2da4929a..a62bee78 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -801,7 +801,7 @@ def _check_postgres(uri: Optional[str]) -> Check: row = cursor.fetchone() current = row[0] if row else None except (psycopg.Error, OSError, ValueError) as exc: - return Check("postgres", "fail", f"cannot connect: {str(exc).strip()}") + return Check("postgres", "fail", f"cannot connect to {_endpoint(uri)}: {_scrub(str(exc).strip(), uri)}") head = _migration_head() if not current: return Check("postgres", "fail", f"PostgreSQL {version}, no schema yet; run `docsgpt migrate`") @@ -811,8 +811,8 @@ def _check_postgres(uri: Optional[str]) -> Check: return Check("postgres", "ok", f"PostgreSQL {version}, schema at {current}") -def _redis_endpoint(url: str) -> str: - """A Redis URL without its credentials: this ends up on a terminal, in CI logs and in issues.""" +def _endpoint(url: str) -> str: + """A URL without its credentials: this ends up on a terminal, in CI logs and in issues.""" try: parts = urlsplit(url) except ValueError: @@ -823,6 +823,21 @@ def _redis_endpoint(url: str) -> str: return f"{parts.scheme}://{host}{parts.path}" if host else "the configured URL" +def _scrub(text: str, url: Optional[str]) -> str: + """Client errors quote the URL they were handed, credentials and all, so take them back out.""" + if not url: + return text + text = text.replace(url, _endpoint(url)) + try: + parts = urlsplit(url) + except ValueError: + return text + for secret in (parts.password, parts.username): + if secret: + text = text.replace(secret, "...") + return text + + def _check_redis(urls: Mapping[str, str]) -> Check: """Ping every Redis the settings name; they are usually one server, three databases.""" if not urls: @@ -835,7 +850,9 @@ def _check_redis(urls: Mapping[str, str]) -> Check: try: redis.Redis.from_url(url, socket_connect_timeout=3).ping() except Exception as exc: # noqa: BLE001 - every client error here is the same finding - return Check("redis", "fail", f"{label} ({_redis_endpoint(url)}) does not answer: {str(exc).strip()}") + return Check( + "redis", "fail", f"{label} ({_endpoint(url)}) does not answer: {_scrub(str(exc).strip(), url)}" + ) return Check("redis", "ok", f"answering on {len(urls)} database(s)") diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 9c73b77e..8e54efc3 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -213,6 +213,22 @@ class TestPostgresCheck: assert check.level == "fail" assert "connection refused" in check.detail + def test_a_database_password_is_never_printed(self, monkeypatch): + """psycopg quotes the connection string it was given, which carries the password.""" + import psycopg + + uri = "postgresql://docsgpt:hunter2@db.example.com:5432/docsgpt" + + def refuse(*args, **kwargs): + raise psycopg.OperationalError(f'connection to "{uri}" failed: timeout expired') + + monkeypatch.setattr(psycopg, "connect", refuse) + check = commands._check_postgres(uri) + assert check.level == "fail" + assert "hunter2" not in check.detail + assert "db.example.com:5432" in check.detail, "the endpoint still has to be identifiable" + assert "timeout expired" in check.detail, "and the reason has to survive the scrubbing" + def test_without_a_uri_at_all(self): check = commands._check_postgres(None) assert check.level == "fail" @@ -251,7 +267,8 @@ class TestRedisCheck: def from_url(cls, url, **kwargs): class Client: def ping(self): - raise ConnectionError("connection refused") + # redis-py quotes the URL it was handed, which is how the credentials got out. + raise ConnectionError(f"no route to {url}") return Client() @@ -263,7 +280,7 @@ class TestRedisCheck: assert "redis.example.com:6380/0" in check.detail, "the endpoint still has to be identifiable" def test_a_malformed_url_is_not_echoed_either(self): - assert commands._redis_endpoint("redis://[::1") == "the configured URL" + assert commands._endpoint("redis://[::1") == "the configured URL" def test_without_any_redis_configured(self): assert commands._check_redis({}).level == "fail" From 1cce6cfd673c5f00627d6fdbab81b5a4333e954d Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 12:02:40 +0100 Subject: [PATCH 06/14] test: cover the two things dev refuses to do Running outside a checkout and starting on a port something else holds are both refusals a developer will meet, and neither had a test. The second also asserts that nothing is spawned when the port is taken, which is the part that matters: the guard runs before any child process exists. --- tests/deploy/test_doctor.py | 49 +++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 8e54efc3..185f8c4f 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -1,5 +1,7 @@ """`docsgpt doctor`, `restart`, following native logs, and settings that apply themselves.""" +import socket + import pytest from docsgpt.deploy import commands, envfile @@ -14,6 +16,53 @@ def _installed_native(tmp_path, services): assert _run(argv, _native_context(services)) == 0 +class TestDevCommand: + def _checkout(self, monkeypatch, tmp_path): + from docsgpt.core import paths + + monkeypatch.setattr(paths, "checkout_root", lambda: tmp_path) + + def _free_port(self): + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: + probe.bind(("127.0.0.1", 0)) + return probe.getsockname()[1] + + def test_an_installed_package_is_pointed_at_native_mode(self, monkeypatch): + """`dev` runs the code you are editing; there is none to edit outside a checkout.""" + from docsgpt.core import paths + + monkeypatch.setattr(paths, "checkout_root", lambda: None) + with pytest.raises(DeployError, match="source checkout"): + _run(["dev"], _context()) + + def test_a_busy_port_is_refused_before_anything_starts(self, monkeypatch, tmp_path): + from docsgpt.deploy import dev as dev_module + + self._checkout(monkeypatch, tmp_path) + started = [] + monkeypatch.setattr(dev_module, "run", lambda children, **kwargs: started.append(children) or 0) + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as held: + held.bind(("127.0.0.1", 0)) + held.listen(1) + port = held.getsockname()[1] + with pytest.raises(DeployError, match="already in use"): + _run(["dev", "--port", str(port)], _context()) + assert started == [], "nothing is spawned when the port is taken" + + def test_it_runs_the_children_it_planned_and_says_where(self, monkeypatch, tmp_path, capsys): + from docsgpt.deploy import dev as dev_module + + self._checkout(monkeypatch, tmp_path) + started = [] + monkeypatch.setattr(dev_module, "run", lambda children, **kwargs: started.append(children) or 0) + port = self._free_port() + assert _run(["dev", "--port", str(port)], _context()) == 0 + assert [child.name for child in started[0]] == ["api", "worker"] + printed = capsys.readouterr().out + assert f"http://127.0.0.1:{port}" in printed + assert "Ctrl-C" in printed + + class TestRestart: def test_it_stops_and_starts_both_without_touching_settings(self, tmp_path): services = FakeServices() From 66fbb110492113b488a98b26dc62d59de1cc8f43 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 12:12:28 +0100 Subject: [PATCH 07/14] fix: doctor survives a Redis URL whose port will not parse urlsplit accepts redis://host:notaport/0; only parts.port raises, and it raises on access rather than at split time, so the ValueError fell outside the try. _check_redis catches the client error and then formats it through _endpoint, so doctor ended with a traceback from inside its own error path. --- docsgpt/deploy/commands.py | 9 +++++---- tests/deploy/test_doctor.py | 22 ++++++++++++++++++++-- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index a62bee78..758fc715 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -815,12 +815,13 @@ def _endpoint(url: str) -> str: """A URL without its credentials: this ends up on a terminal, in CI logs and in issues.""" try: parts = urlsplit(url) + # .port is a property that parses on access, so it raises separately from the split itself. + host, port, scheme, path = parts.hostname or "", parts.port, parts.scheme, parts.path except ValueError: return "the configured URL" - host = parts.hostname or "" - if parts.port: - host = f"{host}:{parts.port}" - return f"{parts.scheme}://{host}{parts.path}" if host else "the configured URL" + if port: + host = f"{host}:{port}" + return f"{scheme}://{host}{path}" if host else "the configured URL" def _scrub(text: str, url: Optional[str]) -> str: diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 185f8c4f..ac046a85 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -328,8 +328,26 @@ class TestRedisCheck: assert "default" not in check.detail assert "redis.example.com:6380/0" in check.detail, "the endpoint still has to be identifiable" - def test_a_malformed_url_is_not_echoed_either(self): - assert commands._endpoint("redis://[::1") == "the configured URL" + @pytest.mark.parametrize("url", ["redis://[::1", "redis://localhost:not-a-port/0"]) + def test_a_malformed_url_is_not_echoed_either(self, url): + """urlsplit raises on the first; on the second it succeeds and .port raises on access.""" + assert commands._endpoint(url) == "the configured URL" + + def test_a_failure_on_a_url_with_an_unparsable_port_is_still_a_check(self, monkeypatch): + """The check catches the client error and then formats it, which is where this raised.""" + import redis + + def from_url(cls, url, **kwargs): + class Client: + def ping(self): + raise ConnectionError(f"no route to {url}") + + return Client() + + monkeypatch.setattr(redis.Redis, "from_url", classmethod(from_url)) + check = commands._check_redis({"broker": "redis://localhost:not-a-port/0"}) + assert check.level == "fail", "a bad port is a finding, not a traceback" + assert "the configured URL" in check.detail def test_without_any_redis_configured(self): assert commands._check_redis({}).level == "fail" From 9e86dd4e94f4c388a6ec61fffebda1cf032d5ceb Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 12:22:45 +0100 Subject: [PATCH 08/14] fix: refuse a busy mock LLM port before dev starts anything The mock starts first and the API and worker are pointed at it, but only the API port was preflighted. A busy 8090 therefore showed up as a child exiting once the rest were running, or as the API talking to whatever else was on that port. --ui is deliberately left alone: vite.config.ts sets no strictPort, so Vite moves to the next free port rather than failing. --- docsgpt/deploy/commands.py | 7 +++++++ tests/deploy/test_doctor.py | 17 +++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 758fc715..b8f063aa 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -739,6 +739,13 @@ def dev(args, context: Optional[Context] = None) -> int: f"port {args.port} is already in use, so the API cannot bind it. Stop what is on it " f"(a previous `docsgpt dev`, or `docsgpt down` for an install), or pass --port." ) + if getattr(args, "mock_llm", False) and not _port_is_free(dev_module.MOCK_LLM_PORT): + # It starts first and the others are pointed at it, so a busy port here would surface as the + # API talking to someone else's server, or as a child exiting once everything else is up. + raise DeployError( + f"port {dev_module.MOCK_LLM_PORT} is already in use, so the mock LLM cannot bind it. " + "Stop what is on it, or leave --mock-llm off and point DocsGPT at a real provider." + ) children = dev_module.plan(args, checkout) print(f"DocsGPT from {checkout}") for child in children: diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index ac046a85..c0a52b28 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -49,6 +49,23 @@ class TestDevCommand: _run(["dev", "--port", str(port)], _context()) assert started == [], "nothing is spawned when the port is taken" + def test_a_busy_mock_llm_port_is_refused_before_anything_starts(self, monkeypatch, tmp_path): + """It starts first and the rest are pointed at it, so a clash cannot wait until spawn time.""" + from docsgpt.deploy import dev as dev_module + + self._checkout(monkeypatch, tmp_path) + (tmp_path / "scripts").mkdir() + (tmp_path / "scripts" / "mock_llm.py").write_text("", encoding="utf-8") + started = [] + monkeypatch.setattr(dev_module, "run", lambda children, **kwargs: started.append(children) or 0) + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as held: + held.bind(("127.0.0.1", 0)) + held.listen(1) + monkeypatch.setattr(dev_module, "MOCK_LLM_PORT", held.getsockname()[1]) + with pytest.raises(DeployError, match="mock LLM cannot bind"): + _run(["dev", "--mock-llm", "--port", str(self._free_port())], _context()) + assert started == [], "nothing is spawned when the mock LLM has nowhere to listen" + def test_it_runs_the_children_it_planned_and_says_where(self, monkeypatch, tmp_path, capsys): from docsgpt.deploy import dev as dev_module From bb50d0a91d3e985183ffefff433bbb430329ccaf Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 12:39:41 +0100 Subject: [PATCH 09/14] fix: refuse to give the API the mock LLM's port Both preflights passed for `dev --mock-llm --port 8090`: the port was free, and then the mock and the API were each handed it. One child could not bind, and the API was pointed at that port as its model server while trying to listen on it. --- docsgpt/deploy/commands.py | 6 ++++++ tests/deploy/test_doctor.py | 14 ++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index b8f063aa..94a144f6 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -739,6 +739,12 @@ def dev(args, context: Optional[Context] = None) -> int: f"port {args.port} is already in use, so the API cannot bind it. Stop what is on it " f"(a previous `docsgpt dev`, or `docsgpt down` for an install), or pass --port." ) + if getattr(args, "mock_llm", False) and args.port == dev_module.MOCK_LLM_PORT: + # Both checks below would pass: the port really is free, and then two children want it. + raise DeployError( + f"port {args.port} is where the mock LLM listens, so the API cannot have it too. " + "Give the API another port with --port." + ) if getattr(args, "mock_llm", False) and not _port_is_free(dev_module.MOCK_LLM_PORT): # It starts first and the others are pointed at it, so a busy port here would surface as the # API talking to someone else's server, or as a child exiting once everything else is up. diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index c0a52b28..4acca1d5 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -66,6 +66,20 @@ class TestDevCommand: _run(["dev", "--mock-llm", "--port", str(self._free_port())], _context()) assert started == [], "nothing is spawned when the mock LLM has nowhere to listen" + def test_the_api_cannot_be_given_the_mock_llm_port(self, monkeypatch, tmp_path): + """Both free-port checks pass here: the port is free, and then two children want it.""" + from docsgpt.deploy import dev as dev_module + + self._checkout(monkeypatch, tmp_path) + (tmp_path / "scripts").mkdir() + (tmp_path / "scripts" / "mock_llm.py").write_text("", encoding="utf-8") + started = [] + monkeypatch.setattr(dev_module, "run", lambda children, **kwargs: started.append(children) or 0) + monkeypatch.setattr(dev_module, "MOCK_LLM_PORT", self._free_port()) + with pytest.raises(DeployError, match="mock LLM listens"): + _run(["dev", "--mock-llm", "--port", str(dev_module.MOCK_LLM_PORT)], _context()) + assert started == [], "nothing is spawned when the two would collide" + def test_it_runs_the_children_it_planned_and_says_where(self, monkeypatch, tmp_path, capsys): from docsgpt.deploy import dev as dev_module From 48285f4405c3604b9bde70672a88a5b88aa98c23 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 12:51:50 +0100 Subject: [PATCH 10/14] fix: doctor looks for alembic_version where alembic puts it env.py sets no version_table_schema, so the table follows search_path. Pinning public. in doctor's queries made them agree with each other and disagree with alembic: on an install using another schema it reported no schema at all and sent the user to migrate an already-migrated database. Both queries resolve the table the same way alembic does now. --- docsgpt/deploy/commands.py | 8 +++++--- tests/deploy/test_doctor.py | 13 +++++++++---- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 94a144f6..ca7be581 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -805,12 +805,14 @@ def _check_postgres(uri: Optional[str]) -> Check: with psycopg.connect(uri, connect_timeout=5) as connection, connection.cursor() as cursor: cursor.execute("select current_setting('server_version')") version = cursor.fetchone()[0] - cursor.execute("select to_regclass('public.alembic_version')") + # Unqualified, like alembic itself: env.py sets no version_table_schema, so the table + # lives wherever search_path puts it. Asserting public would call a migrated database empty. + cursor.execute("select to_regclass('alembic_version')") applied = cursor.fetchone()[0] is not None current = None if applied: - # public, like the to_regclass check above: search_path could resolve another one. - cursor.execute("select version_num from public.alembic_version") + # The same relation the check above resolved, by the same rules. + cursor.execute("select version_num from alembic_version") row = cursor.fetchone() current = row[0] if row else None except (psycopg.Error, OSError, ValueError) as exc: diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 4acca1d5..4cc4a343 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -261,12 +261,17 @@ class TestPostgresCheck: assert check.level == "ok" assert "16.2" in check.detail and "0031_x" in check.detail - def test_the_revision_comes_from_the_schema_that_was_checked(self, monkeypatch): - """to_regclass looks in public, so the second query must not resolve through search_path.""" + def test_both_queries_resolve_the_same_table(self, monkeypatch): + """Alembic sets no version_table_schema, so the table follows search_path; asserting a + schema in one query and not the other is how doctor called a migrated database empty.""" cursor = _postgres_answering(monkeypatch) commands._check_postgres("postgresql://localhost/d") - revision_query = [statement for statement in cursor.statements if "version_num" in statement] - assert revision_query and all("public.alembic_version" in statement for statement in revision_query) + looked_up = [statement for statement in cursor.statements if "to_regclass" in statement] + read = [statement for statement in cursor.statements if "version_num" in statement] + assert looked_up and read + assert all("public." not in statement for statement in looked_up + read), ( + "neither query may pin a schema alembic never promised" + ) def test_a_database_with_no_schema_yet(self, monkeypatch): """The commonest first-run state: the database exists, nothing has been migrated into it.""" From d0a0b352e0dd5003a380563190a8b9de0a22e263 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 13:03:59 +0100 Subject: [PATCH 11/14] fix: drop the host-substring assertions and explain the empty except CodeQL flags `"host:port" in message` as incomplete URL sanitisation. It is a message rather than a URL being authorised, so it is not a vulnerability, but the check was red and comparing against the sanitiser's own output asserts the real contract. The signal handler now says why it swallows: the child is already gone, and shutdown must not fail on what it is cleaning up. --- docsgpt/deploy/dev.py | 2 ++ tests/deploy/test_doctor.py | 9 ++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/docsgpt/deploy/dev.py b/docsgpt/deploy/dev.py index a49dcf07..a552646e 100644 --- a/docsgpt/deploy/dev.py +++ b/docsgpt/deploy/dev.py @@ -143,6 +143,8 @@ def _signal(process, number: int) -> None: return os.killpg(os.getpgid(process.pid), number) except (ProcessLookupError, PermissionError, OSError): + # The child has already gone, or its group is no longer ours to signal. Either way there is + # nothing left to stop, and shutdown must not fail on the thing it is trying to clean up. pass diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 4cc4a343..1a55ce84 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -311,7 +311,9 @@ class TestPostgresCheck: check = commands._check_postgres(uri) assert check.level == "fail" assert "hunter2" not in check.detail - assert "db.example.com:5432" in check.detail, "the endpoint still has to be identifiable" + # Compared against what the sanitiser produced, not a host substring: asking whether a URL + # contains a host is the check CodeQL warns about, and it is not what this test means. + assert check.detail.startswith(f"cannot connect to {commands._endpoint(uri)}") assert "timeout expired" in check.detail, "and the reason has to survive the scrubbing" def test_without_a_uri_at_all(self): @@ -358,11 +360,12 @@ class TestRedisCheck: return Client() monkeypatch.setattr(redis.Redis, "from_url", classmethod(from_url)) - check = commands._check_redis({"broker": "rediss://default:sUpErSeCrEt@redis.example.com:6380/0"}) + url = "rediss://default:sUpErSeCrEt@redis.example.com:6380/0" + check = commands._check_redis({"broker": url}) assert check.level == "fail" assert "sUpErSeCrEt" not in check.detail assert "default" not in check.detail - assert "redis.example.com:6380/0" in check.detail, "the endpoint still has to be identifiable" + assert check.detail.startswith(f"broker ({commands._endpoint(url)}) does not answer") @pytest.mark.parametrize("url", ["redis://[::1", "redis://localhost:not-a-port/0"]) def test_a_malformed_url_is_not_echoed_either(self, url): From da58c072a0ff69b334dc64366f3089fe8cc492ee Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 13:15:56 +0100 Subject: [PATCH 12/14] fix: five from the full review - doctor printed OPENAI_BASE_URL raw, the third place a credential-bearing URL reached the terminal; it goes through _endpoint like the others. - dev.run left its output readers unjoined, so a child's last lines could be lost on exit. The threads are kept and joined during teardown. - logs read each file and then reopened it to follow, so anything written in between appeared in neither. One handle now serves both. - dev took --port straight from argparse: 0 would have served on an ephemeral port while printing 0, and oversized values reach socket.bind. It goes through _port_number first. - doctor --redis-url overrode only the broker, so a stale result backend or cache was still pinged and the flag looked broken. All three endpoints now come from the URL given. --- docsgpt/deploy/commands.py | 58 +++++++++++++++++++++--------- docsgpt/deploy/dev.py | 9 ++++- tests/deploy/test_dev.py | 11 ++++++ tests/deploy/test_doctor.py | 71 +++++++++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 18 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index ca7be581..25a7d9a0 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -615,24 +615,35 @@ def logs(args, context: Optional[Context] = None) -> int: if _mode(directory) == "native": logs_dir = directory / "logs" wanted = args.services or ["api", "worker"] + # Opened before the first read and kept: a line written between printing what is there and + # starting to follow would otherwise appear in neither. + handles = {} for service in wanted: path = logs_dir / f"{service}.log" print(f"=== {path}") - if path.is_file(): - lines = path.read_text(encoding="utf-8", errors="replace").splitlines() - print("\n".join(lines[-args.tail:] if args.tail else lines)) - else: + if not path.is_file(): print("(nothing logged yet)") + continue + handle = path.open("r", encoding="utf-8", errors="replace") + lines = handle.read().splitlines() + print("\n".join(lines[-args.tail:] if args.tail else lines)) + handles[service] = handle if args.follow: - return _follow(logs_dir, wanted) + return _follow(logs_dir, wanted, handles) + for handle in handles.values(): + handle.close() return 0 options = (["--follow"] if args.follow else []) + (["--tail", str(args.tail)] if args.tail else []) return context.docker.compose(directory, "logs", *options, *args.services, check=False).returncode -def _follow(logs_dir: Path, services: list) -> int: - """Print new lines from each service's log until the terminal interrupts, prefixed by service.""" - handles: dict = {} +def _follow(logs_dir: Path, services: list, handles: Optional[dict] = None) -> int: + """Print new lines from each service's log until the terminal interrupts, prefixed by service. + + ``handles`` are the files already read, positioned where that read left off, so nothing written + in between is skipped. Files that did not exist yet are opened as they appear. + """ + handles = dict(handles or {}) try: while True: for service in services: @@ -734,7 +745,8 @@ def dev(args, context: Optional[Context] = None) -> int: "`docsgpt dev` runs the code in a source checkout, and this is an installed package. " "Clone the repository and run it from there, or use `docsgpt up --native` to run this copy." ) - if not _port_is_free(args.port): + port = _port_number(args.port, "--port") + if not _port_is_free(port): raise DeployError( f"port {args.port} is already in use, so the API cannot bind it. Stop what is on it " f"(a previous `docsgpt dev`, or `docsgpt down` for an install), or pass --port." @@ -879,7 +891,25 @@ def _check_provider(env: Mapping[str, str]) -> Check: return Check("provider", "ok", "the DocsGPT public API (no key needed)") if not (env.get("API_KEY") or env.get("OPENAI_API_KEY")): return Check("provider", "fail", f"{provider} is configured but no API_KEY is set") - return Check("provider", "ok", f"{provider}{' at ' + env['OPENAI_BASE_URL'] if env.get('OPENAI_BASE_URL') else ''}") + endpoint = _endpoint(env["OPENAI_BASE_URL"]) if env.get("OPENAI_BASE_URL") else "" + return Check("provider", "ok", f"{provider}{' at ' + endpoint if endpoint else ''}") + + +def _redis_to_check(args, env: Mapping[str, str]) -> dict: + """The Redis endpoints to ping: all three from --redis-url when given, else what .env holds. + + Overriding only the broker would still ping a stale result backend or cache, and the check + fails on the first endpoint that does not answer -- so --redis-url would appear not to work. + """ + if args.redis_url: + names = {"CELERY_BROKER_URL": "broker", "CELERY_RESULT_BACKEND": "results", "CACHE_REDIS_URL": "cache"} + return {names[key]: value for key, value in _redis_urls(args.redis_url).items()} + pairs = ( + ("broker", env.get("CELERY_BROKER_URL")), + ("results", env.get("CELERY_RESULT_BACKEND")), + ("cache", env.get("CACHE_REDIS_URL")), + ) + return {key: value for key, value in pairs if value} def doctor(args, context: Optional[Context] = None) -> int: @@ -898,13 +928,7 @@ def doctor(args, context: Optional[Context] = None) -> int: Check("settings", "ok" if env_path.is_file() else "warn", f"{env_path}" if env_path.is_file() else f"{env_path} does not exist yet; defaults are in use"), _check_postgres(args.postgres_uri or env.get("POSTGRES_URI")), - _check_redis({ - key: value for key, value in ( - ("broker", args.redis_url or env.get("CELERY_BROKER_URL")), - ("results", env.get("CELERY_RESULT_BACKEND")), - ("cache", env.get("CACHE_REDIS_URL")), - ) if value - }), + _check_redis(_redis_to_check(args, env)), _check_provider(env), ] diff --git a/docsgpt/deploy/dev.py b/docsgpt/deploy/dev.py index a552646e..18c4e485 100644 --- a/docsgpt/deploy/dev.py +++ b/docsgpt/deploy/dev.py @@ -181,6 +181,7 @@ def run( colour = out.isatty() if colour is None else colour lock = threading.Lock() running: list[tuple[Child, object]] = [] + pumps: list[threading.Thread] = [] try: for child in children: process = spawn( @@ -195,7 +196,9 @@ def run( start_new_session=os.name != "nt", ) running.append((child, process)) - threading.Thread(target=_pump, args=(child, process, out, lock, colour), daemon=True).start() + pump = threading.Thread(target=_pump, args=(child, process, out, lock, colour), daemon=True) + pump.start() + pumps.append(pump) while True: for child, process in running: @@ -214,3 +217,7 @@ def run( return 0 finally: _stop(running, grace, sleep) + # Join the readers: a child's last lines are still in flight when it exits, and dropping + # them loses exactly the output that says why it stopped. + for pump in pumps: + pump.join(timeout=grace) diff --git a/tests/deploy/test_dev.py b/tests/deploy/test_dev.py index a8344954..5426cbd6 100644 --- a/tests/deploy/test_dev.py +++ b/tests/deploy/test_dev.py @@ -123,6 +123,17 @@ class TestRun: assert "api | hello" in out.getvalue() assert code == 1, "a child that ends by itself ends the session, however it exited" + def test_output_in_flight_is_not_lost_when_run_returns(self, tmp_path): + """The reader is a thread: without joining it, a child's last lines can never be printed.""" + out = io.StringIO() + lines = [f"line {number}\n" for number in range(200)] + children = [dev.Child(name="api", command=["x"], cwd=tmp_path)] + code = dev.run(children, out=out, spawn=self._spawn([FakeProcess(lines, code=0)]), + sleep=lambda _: None, colour=False) + assert code == 1 + printed = out.getvalue() + assert "line 0" in printed and "line 199" in printed, "every line the child wrote is printed" + def test_a_failing_child_returns_its_code(self, tmp_path): out = io.StringIO() children = [dev.Child(name="api", command=["false"], cwd=tmp_path)] diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 1a55ce84..252af2fa 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -1,5 +1,6 @@ """`docsgpt doctor`, `restart`, following native logs, and settings that apply themselves.""" +import argparse import socket import pytest @@ -49,6 +50,18 @@ class TestDevCommand: _run(["dev", "--port", str(port)], _context()) assert started == [], "nothing is spawned when the port is taken" + @pytest.mark.parametrize("port", ["0", "-1", "70000"]) + def test_a_port_argparse_accepts_but_a_socket_cannot_use(self, monkeypatch, tmp_path, port): + """argparse takes any integer: 0 would serve on an ephemeral port while printing 0.""" + from docsgpt.deploy import dev as dev_module + + self._checkout(monkeypatch, tmp_path) + started = [] + monkeypatch.setattr(dev_module, "run", lambda children, **kwargs: started.append(children) or 0) + with pytest.raises(DeployError, match="not a port number"): + _run(["dev", "--port", port], _context()) + assert started == [] + def test_a_busy_mock_llm_port_is_refused_before_anything_starts(self, monkeypatch, tmp_path): """It starts first and the rest are pointed at it, so a clash cannot wait until spawn time.""" from docsgpt.deploy import dev as dev_module @@ -184,6 +197,33 @@ class TestFollowLogs: assert "old line" not in printed, "it starts at the end, like tail -f" +class TestLogsTransition: + def test_a_line_written_between_the_read_and_the_follow_is_not_lost(self, tmp_path, capsys, monkeypatch): + """The read and the follow used to open the file twice, and whatever landed between was gone.""" + services = FakeServices() + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] + assert _run(argv, _native_context(services)) == 0 + logs = tmp_path / "logs" + (logs / "api.log").write_text("first\n", encoding="utf-8") + + def sleep(_): + raise KeyboardInterrupt + + original = commands._follow + + def follow(logs_dir, wanted, handles=None): + # Written after the read, before following starts. + (logs / "api.log").open("a", encoding="utf-8").write("during\n") + monkeypatch.setattr(commands.time, "sleep", sleep) + return original(logs_dir, wanted, handles) + + monkeypatch.setattr(commands, "_follow", follow) + assert _run(["logs", "-f", "api", "--dir", str(tmp_path)], _native_context(services)) == 0 + printed = capsys.readouterr().out + assert "first" in printed + assert "during" in printed, "the line written during the handover has to appear" + + class TestChecks: def test_the_public_api_needs_no_key(self): check = commands._check_provider({"LLM_PROVIDER": "docsgpt"}) @@ -201,6 +241,17 @@ class TestChecks: assert check.level == "ok" assert "8090" in check.detail + def test_a_provider_base_url_is_printed_without_its_credentials(self): + """An OpenAI-compatible endpoint can carry userinfo, and doctor prints its detail.""" + check = commands._check_provider( + {"LLM_PROVIDER": "openai", "API_KEY": "x", + "OPENAI_BASE_URL": "https://someone:sEcReTtOkEn@models.example.com/v1"} + ) + assert check.level == "ok" + assert "sEcReTtOkEn" not in check.detail + assert "someone" not in check.detail + assert "models.example.com" in check.detail + def test_services_are_named_for_the_install(self, tmp_path): names = _names(tmp_path) assert commands._chosen_services(names, []) == list(names) @@ -400,6 +451,26 @@ class TestMigrationHead: assert head[0].isdigit(), head +class TestRedisSelection: + def test_redis_url_replaces_every_endpoint(self): + """Overriding only the broker would still ping a stale backend, and one failure fails all.""" + args = argparse.Namespace(redis_url="redis://given:6379/5") + stale = { + "CELERY_BROKER_URL": "redis://old:6379/0", + "CELERY_RESULT_BACKEND": "redis://old:6379/1", + "CACHE_REDIS_URL": "redis://old:6379/2", + } + chosen = commands._redis_to_check(args, stale) + assert set(chosen) == {"broker", "results", "cache"} + assert all("old" not in url for url in chosen.values()), chosen + assert chosen["broker"].endswith("/5") and chosen["cache"].endswith("/7") + + def test_without_the_flag_the_settings_are_used(self): + args = argparse.Namespace(redis_url=None) + chosen = commands._redis_to_check(args, {"CELERY_BROKER_URL": "redis://localhost:6379/0"}) + assert chosen == {"broker": "redis://localhost:6379/0"} + + class TestDoctor: def _only(self, monkeypatch, postgres, redis): monkeypatch.setattr(commands, "_check_postgres", lambda uri: postgres) From 6f981a559814b7c556c942fc6bc26250240e6d04 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 13:19:34 +0100 Subject: [PATCH 13/14] test: compare against the sanitiser, not a host substring CodeQL flags "host" in message as incomplete URL sanitisation. I removed that shape from two assertions and introduced a third in the same commit; this takes it out of the provider test and the remaining Redis one, comparing with what _endpoint produced instead, which is the contract those tests actually mean. --- tests/deploy/test_doctor.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 252af2fa..eb085038 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -243,14 +243,14 @@ class TestChecks: def test_a_provider_base_url_is_printed_without_its_credentials(self): """An OpenAI-compatible endpoint can carry userinfo, and doctor prints its detail.""" - check = commands._check_provider( - {"LLM_PROVIDER": "openai", "API_KEY": "x", - "OPENAI_BASE_URL": "https://someone:sEcReTtOkEn@models.example.com/v1"} - ) + url = "https://someone:sEcReTtOkEn@models.example.com/v1" + check = commands._check_provider({"LLM_PROVIDER": "openai", "API_KEY": "x", "OPENAI_BASE_URL": url}) assert check.level == "ok" assert "sEcReTtOkEn" not in check.detail assert "someone" not in check.detail - assert "models.example.com" in check.detail + # Compared with what the sanitiser produced rather than a host substring, which is both the + # real contract and the pattern CodeQL warns about. + assert check.detail == f"openai at {commands._endpoint(url)}" def test_services_are_named_for_the_install(self, tmp_path): names = _names(tmp_path) @@ -394,9 +394,10 @@ class TestRedisCheck: return Client() monkeypatch.setattr(redis.Redis, "from_url", classmethod(from_url)) - check = commands._check_redis({"cache": "redis://localhost:6379/2"}) + url = "redis://localhost:6379/2" + check = commands._check_redis({"cache": url}) assert check.level == "fail" - assert "cache" in check.detail and "6379/2" in check.detail + assert check.detail.startswith(f"cache ({commands._endpoint(url)}) does not answer") def test_a_password_in_the_url_is_never_printed(self, monkeypatch): """doctor output goes into terminals, CI logs and pasted issue reports.""" From 8b064c96f25dcc4e9db08a4bde215089aaf92630 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 13:34:59 +0100 Subject: [PATCH 14/14] fix: keep credentials and 5000-digit paths out of the Redis URL errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _redis_urls raises before any check runs and cli.main prints what it raises, so every one of its five messages interpolated the URL it was given — password and all — straight to stderr. They render it through _endpoint now. _endpoint itself then echoed the whole path, so an over-long database number came back at full length: 5132 characters of error for one bad setting. It caps the path it renders, which protects every caller, since that string is written into terminals, CI logs and error messages rather than reused as a URL. --- docsgpt/deploy/commands.py | 18 ++++++++++++------ tests/deploy/test_doctor.py | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 25a7d9a0..295ae0ac 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -215,30 +215,32 @@ def _redis_urls(base: str) -> dict[str, str]: parts = urlsplit(base) except ValueError as exc: # urlsplit raises on things like redis://[::1 ; that is a typo, not a crash. - raise DeployError(f"the Redis URL {base!r} could not be read: {exc}") from exc + raise DeployError(f"the Redis URL could not be read: {_scrub(str(exc), base)}") from exc try: port = parts.port # a non-numeric or out-of-range port raises here, not when the URL is split except ValueError as exc: raise DeployError( - f"the Redis URL {base!r} has an unusable port: {exc}. Pass a URL like " + f"the Redis URL ({_endpoint(base)}) has an unusable port: {_scrub(str(exc), base)}. " + "Pass a URL like " "redis://host:6379 or redis://host:6379/5." ) from exc if port == 0: # urlsplit is happy with it, since 0 is inside the range, but nothing can connect to it. raise DeployError( - f"the Redis URL {base!r} has an unusable port: 0. Pass a URL like " + f"the Redis URL ({_endpoint(base)}) has an unusable port: 0. Pass a URL like " "redis://host:6379 or redis://host:6379/5." ) if parts.scheme not in ("redis", "rediss"): raise DeployError( - f"the Redis URL {base!r} should start with redis:// or rediss://, with any options as " + f"the Redis URL ({_endpoint(base)}) should start with redis:// or rediss://, with any options as " "query parameters, so the broker, the result backend and the cache can be given a " "database each." ) path = parts.path.rstrip("/").lstrip("/") if path and not (path.isascii() and path.isdigit()): raise DeployError( - f"the Redis URL {base!r} has {path!r} where a database number would go. Pass a URL like " + f"the Redis URL ({_endpoint(base)}) has {path[:20]!r} where a database number would go. " + "Pass a URL like " "redis://host:6379 or redis://host:6379/5." ) try: @@ -247,7 +249,7 @@ def _redis_urls(base: str) -> dict[str, str]: # Python refuses to convert a digit string past its conversion limit, and that is a typo # rather than a crash. raise DeployError( - f"the Redis URL {base!r} has a database number too long to read. Pass a URL like " + f"the Redis URL ({_endpoint(base)}) has a database number too long to read. Pass a URL like " "redis://host:6379 or redis://host:6379/5." ) from exc return { @@ -848,6 +850,10 @@ def _endpoint(url: str) -> str: return "the configured URL" if port: host = f"{host}:{port}" + # The path is capped: this string is for a person to read, and it goes into terminals, CI logs + # and error messages. A 5000-digit database number would otherwise flood all three. + if len(path) > 40: + path = f"{path[:40]}..." return f"{scheme}://{host}{path}" if host else "the configured URL" diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index eb085038..e3072249 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -472,6 +472,28 @@ class TestRedisSelection: assert chosen == {"broker": "redis://localhost:6379/0"} +class TestRedisUrlErrors: + """The validator raises before any check runs, and cli.main prints what it raises.""" + + SECRET = "sUpErSeCrEt" + + @pytest.mark.parametrize("url", [ + "redis://user:sUpErSeCrEt@host:not-a-port/0", + "redis://user:sUpErSeCrEt@host:0/0", + "redis://user:sUpErSeCrEt@host:6379/queue", + "postgres://user:sUpErSeCrEt@host:6379/0", + "redis://user:sUpErSeCrEt@host:6379/" + "1" * 5000, + ]) + def test_a_rejected_url_never_carries_its_password_into_the_error(self, tmp_path, url): + (tmp_path / ".env").write_text("LLM_PROVIDER=docsgpt\n", encoding="utf-8") + with pytest.raises(DeployError) as raised: + _run(["doctor", "--dir", str(tmp_path), "--redis-url", url], _context()) + message = str(raised.value) + assert self.SECRET not in message + assert "user" not in message + assert len(message) < 400, "an over-long database number must not come back in the message" + + class TestDoctor: def _only(self, monkeypatch, postgres, redis): monkeypatch.setattr(commands, "_check_postgres", lambda uri: postgres)