fix(cli): keel open says WHERE it looked, because that was the missing fact

keel/commands/open_console.py

@@ -19,6 +19,7 @@
 from __future__ import annotations
 
 import webbrowser
+from typing import NoReturn
 
 import click
 
@@ -41,11 +42,15 @@
     help="Print the address without launching a browser.",
 )
 def open_cmd(port: int, no_browser: bool) -> None:
-    """Print (and open) the address of a running `keel serve`, token included."""
+    """Print (and open) the address of a running `keel serve`, token included.
+
+    The deployment is the one the CURRENT DIRECTORY belongs to (or `KEEL_HOME`), the same way
+    every other bare invocation resolves state -- so run this from the deployment, not from a
+    checkout of the source.
+    """
     record = runtime.live_record(port)
     if record is None:
         _refuse(port)
-        return
 
     url = runtime.url_for(record)
     click.echo(f"Opening the keel console at:\n\n    {url}\n")
@@ -62,25 +67,56 @@ def open_cmd(port: int, no_browser: bool) -> None:
         click.echo(f"(could not launch a browser: {exc})")
 
 
-def _refuse(port: int) -> None:
+def _refuse(port: int) -> NoReturn:
     """Say which of the three absences this is, and what to do about each.
 
     A stale record is reported as a STOPPED server rather than as no server: the file is evidence
     that one ran, and telling an operator "nothing is serving" when a crashed process left its
     record behind hides the thing they most need to know.
     """
+    # WHERE IT LOOKED, in every branch. The first real install of the console daemons succeeded
+    # and read as a failure: `keel open` was run from a source checkout, which is itself a
+    # deployment root (it has a `keel.db` and a `config.yaml`), so it searched that tree while
+    # four healthy servers wrote to `~/keel/run/`. The message listed three explanations and the
+    # true one was not among them -- it could not be, because it never said where it had looked.
+    #
+    # This command takes no `--db` and no `--config`, so nothing in its signature hints that the
+    # answer depends on the working directory. The path is the hint.
+    searched = runtime.run_dir()
+
     stale = runtime.read_record(port)
     if stale is not None:
+        # WHICH of the two liveness checks failed. `live_record` requires a live pid AND something
+        # answering on the port, and this branch used to assert the first had failed regardless --
+        # measured saying "its process is gone" about the very process printing the sentence. A
+        # server that is alive but not yet listening, or bound to a host other than the one
+        # recorded, is a bind problem, and telling its operator it crashed sends them to the wrong
+        # investigation. This codebase refuses that kind of confident wrong claim elsewhere
+        # (`_session_banner` renders nothing rather than name a mode it cannot verify).
+        pid = runtime.recorded_pid(stale)
+        if runtime.process_alive(pid):
+            raise click.ClickException(
+                f"a keel server was recorded on port {port} and its process ({pid}) is still "
+                f"running, but nothing answers on {stale.get('host', '?')}:{port}.\n\n"
+                "  That is a server that failed to bind, or one still starting. Its log says "
+                f"which: `Address already in use` is the common one.\n\n"
+                f"  The record is in {searched}."
+            )
         raise click.ClickException(
-            f"a keel server was recorded on port {port} but its process is gone -- it crashed or "
-            "was killed. Its token died with it; start a new one with `keel serve` (or "
-            "`launchctl kickstart` the agent that runs it)."
+            f"a keel server was recorded on port {port} but its process ({pid}) is gone -- it "
+            "crashed or was killed. Its token died with it; start a new one with `keel serve` "
+            f"(or `launchctl kickstart` the agent that runs it).\n\n"
+            f"  The record is in {searched}."
         )
     raise click.ClickException(
         f"no recorded keel server on port {port}.\n\n"
-        "  If one is running, it was started from a terminal -- an interactive `keel serve` "
+        f"  Looked in {searched} -- the deployment resolved from $KEEL_HOME if that is set, "
+        "and otherwise from the current directory. If your server is a different deployment, run "
+        "this from ITS directory: `keel open` takes the same bare-invocation path as everything "
+        "else, so a source checkout resolves to the checkout.\n\n"
+        "  If one is running here, it was started from a terminal -- an interactive `keel serve` "
         "deliberately records nothing, and its URL is printed in that terminal. Only a detached "
-        "server (launchd, or any run whose stdout is not a terminal) leaves a record here, "
-        "because that is the case where nobody can read the printed line.\n\n"
+        "server (launchd, or any run whose stdout is not a terminal) leaves a record, because "
+        "that is the case where nobody can read the printed line.\n\n"
         f"  If none is running, start one: `keel serve --port {port}`."
     )

keel/web/runtime.py

@@ -173,9 +173,32 @@ def read_record(port: int) -> dict[str, Any] | None:
     return parsed
 
 
-def _process_alive(pid: int) -> bool:
+def recorded_pid(record: dict[str, Any]) -> int:
+    """The pid a record claims, or `0` when it claims nothing usable.
+
+    ONE conversion, because two is how a bug got in (#763 review). `live_record` wrapped its copy
+    in `try`/`except` and `_refuse` repeated it bare, so a record carrying `"pid": "not-a-number"`
+    -- which `read_record` admits, since it validates only that a token is present -- raised
+    `ValueError` out of the refusal path. That is the one path whose entire job is to fail
+    gracefully.
+
+    `0` is the safe answer for anything unparseable: `process_alive` refuses it before signalling
+    (`os.kill(0, ...)` would hit this process's own group), so an unreadable pid reports as not
+    running rather than as anything else.
+    """
+    try:
+        return int(record.get("pid", 0) or 0)
+    except TypeError, ValueError, OverflowError:
+        return 0
+
+
+def process_alive(pid: int) -> bool:
     """Is `pid` a process this user could signal?
 
+    Public since #763: `keel open`'s refusal needs it to tell a server that CRASHED from one
+    that is alive but not answering on its port -- two different investigations, and asserting
+    the wrong one sends an operator looking in the wrong place.
+
     `signal 0` is the standard existence check: it validates the pid and permissions without
     delivering anything. `pid <= 0` is refused before the call, because `os.kill(0, 0)` signals
     the whole process GROUP -- which on a bad record would be this process, and would report the
@@ -225,11 +248,7 @@ def live_record(port: int) -> dict[str, Any] | None:
     record = read_record(port)
     if record is None:
         return None
-    try:
-        pid = int(record.get("pid", 0))
-    except TypeError, ValueError:
-        return None
-    if not _process_alive(pid):
+    if not process_alive(recorded_pid(record)):
         return None
     # AND something must answer on the port (#759 review). A pid check alone is not liveness: keel
     # dies, the OS hands that pid to anything else, and the record reads as live -- so `keel open`

tests/web/test_open_command.py

@@ -8,6 +8,7 @@
 
 from __future__ import annotations
 
+import json
 import os
 from pathlib import Path
 
@@ -310,3 +311,123 @@ def _mine(_signum: int, _frame: object) -> None:
         assert signal.getsignal(signal.SIGTERM) is _mine, "serve kept the handler it installed"
     finally:
         signal.signal(signal.SIGTERM, previous)
+
+
+# -- saying WHERE it looked (found by an install that worked) -------------------------------------
+
+
+def test_the_refusal_names_the_directory_it_searched(home: Path) -> None:
+    """The reason the first real install read as a failure when it had actually succeeded.
+
+    `keel open` resolves the deployment from the CURRENT DIRECTORY (`state_root`), and the dev
+    repo is itself a deployment root -- it has a `keel.db` and a `config.yaml`. So a command
+    chained as `cd <repo> && ... && keel open` searched `<repo>/run/` while four healthy daemons
+    were writing to `~/keel/run/`. The message offered three explanations and not the true one,
+    because none of them could be: it never said where it had looked.
+
+    A path in the refusal turns that from a hunt into a glance.
+    """
+    port = _a_closed_port()
+    result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"])
+    assert result.exit_code != 0
+    assert str(runtime.run_dir()) in result.output, result.output
+
+
+def test_the_stale_refusal_names_it_too(home: Path) -> None:
+    """The other branch. A crashed server and a wrong directory are different problems and an
+    operator staring at either one needs the same fact to tell them apart."""
+    port = _a_closed_port()
+    runtime.record_serving(host="127.0.0.1", port=port, token="Z9-stale-Z9", interactive=False)
+    path = runtime.record_path(port)
+    path.write_text(path.read_text().replace('"pid": ' + str(os.getpid()), '"pid": 0'))
+    result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"])
+    assert result.exit_code != 0
+    assert str(runtime.run_dir()) in result.output, result.output
+    assert "Z9-stale-Z9" not in result.output, "a stale token was printed anyway"
+
+
+def test_the_help_says_the_deployment_comes_from_the_working_directory() -> None:
+    """`keel open` takes no `--db` and no `--config`, so nothing in its signature hints that the
+    answer depends on where you are standing. That is invisible until it bites."""
+    result = CliRunner().invoke(cli, ["open", "--help"])
+    assert result.exit_code == 0
+    lowered = result.output.lower()
+    assert "directory" in lowered or "deployment" in lowered, result.output
+
+
+def test_a_live_pid_that_is_not_listening_is_not_reported_as_dead(home: Path) -> None:
+    """`live_record` has had TWO failure modes since the port probe arrived, and the refusal knew
+    about one.
+
+    Measured against a record whose pid was the running test process: the message said "its
+    process is gone -- it crashed or was killed" about the very process printing it. A server that
+    is alive but not yet listening, or bound to a host other than the one recorded, got told it
+    had crashed -- pointing the operator at the wrong investigation entirely.
+    """
+    port = _a_closed_port()
+    runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False)
+    result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"])
+    assert result.exit_code != 0
+    assert "crashed" not in result.output, result.output
+    assert str(os.getpid()) in result.output, "the message does not name the pid it checked"
+    assert str(port) in result.output
+
+
+def test_a_dead_pid_still_reads_as_a_server_that_stopped(home: Path) -> None:
+    """The other branch, and the one that must not be lost while fixing the first."""
+    port = _a_closed_port()
+    runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False)
+    path = runtime.record_path(port)
+    path.write_text(path.read_text().replace('"pid": ' + str(os.getpid()), '"pid": 0'))
+    result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"])
+    assert result.exit_code != 0
+    assert "crashed" in result.output or "gone" in result.output, result.output
+
+
+def test_the_refusal_is_typed_as_never_returning() -> None:
+    """Both branches raise, so the `return` that used to follow the call was unreachable.
+    `NoReturn` lets a type checker prove that rather than a reader assuming it."""
+    import typing
+
+    from keel.commands.open_console import _refuse
+
+    assert typing.get_type_hints(_refuse).get("return") is typing.NoReturn
+
+
+def test_the_refusal_does_not_claim_the_path_came_from_the_directory_when_it_may_not(
+    home: Path,
+) -> None:
+    """`state_root` honours `KEEL_HOME` FIRST, and this fixture sets it -- so "the deployment this
+    directory belongs to" was false for exactly the reader most likely to have set it."""
+    port = _a_closed_port()
+    result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"])
+    assert "KEEL_HOME" in result.output, result.output
+
+
+def test_a_malformed_pid_refuses_cleanly_instead_of_raising(home: Path) -> None:
+    """The refusal path is the one that must never traceback, and it was the one that did.
+
+    `live_record` wrapped the pid conversion in `try/except (TypeError, ValueError)`; `_refuse`
+    repeated the conversion bare, and `read_record` validates only that a token is present. So a
+    record carrying `"pid": "not-a-number"` produced `ValueError: invalid literal for int()` out
+    of the code whose docstring promises the opposite -- "`keel open` must not traceback at an
+    operator whose server has just died".
+    """
+    port = _a_closed_port()
+    runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False)
+    path = runtime.record_path(port)
+    path.write_text(json.dumps({**json.loads(path.read_text()), "pid": "not-a-number"}))
+
+    result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"])
+    assert result.exception is None or isinstance(result.exception, SystemExit), result.exception
+    assert result.exit_code != 0
+    assert "Error:" in result.output
+
+
+def test_one_place_parses_the_recorded_pid(home: Path) -> None:
+    """Two conversions of one field is how the bug above existed: `live_record` guarded its copy
+    and `_refuse` did not. `recorded_pid` is the single answer both ask for."""
+    assert runtime.recorded_pid({"pid": 42}) == 42
+    assert runtime.recorded_pid({"pid": "7"}) == 7
+    for junk in ({}, {"pid": None}, {"pid": "not-a-number"}, {"pid": [1]}, {"pid": 1.5e400}):
+        assert runtime.recorded_pid(junk) == 0, junk