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

@@ -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),
     ]
 

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)

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)]

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)