From 76f61f4eb5704939e170b8a3f7a540afe87fcff8 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 8 Sep 2026 02:11:16 -0400 Subject: [PATCH 1/3] fix(cli): `keel open` says WHERE it looked, because that was the missing fact Found by an install that worked. Four console daemons bootstrapped, three wrote their records, and `keel open --port 8765` answered "no recorded keel server" -- so a success read as a failure. The command had been run from a source checkout, which is ITSELF a deployment root (`is_deployment_root` is deliberately generous, and the tree has a `keel.db` and a `config.yaml`). So it searched the checkout's `run/` while the servers wrote to `~/keel/run/`. The message offered three explanations and the true one was not among them. It could not be: the message never said where it had looked. Every refusal now names the directory, and the wording says why that directory was chosen. `keel open` 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, and the `--help` says the same thing in one line. The stale-record branch gets it too. A crashed server and a wrong directory are different problems, and an operator staring at either needs the same fact to tell them apart. Verified by running the command from the wrong tree and reading what it prints: it names the worktree it searched, which is exactly the sentence that would have ended the original confusion in a glance. 6,413 passed / 3 skipped, ruff and mypy clean. Three mutants killed: the path dropped from each refusal branch, and the help line removed. Recorded while here, since #756 rests on it: launchd DOES give `keel serve` a stdout that is not a tty. Three daemons, three records written. That was the one assumption nothing in CI could test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/commands/open_console.py | 30 ++++++++++++++++++++---- tests/web/test_open_command.py | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/keel/commands/open_console.py b/keel/commands/open_console.py index 539f2ab..5fab83b 100644 --- a/keel/commands/open_console.py +++ b/keel/commands/open_console.py @@ -41,7 +41,12 @@ 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) @@ -69,18 +74,33 @@ def _refuse(port: int) -> None: 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: 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"`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}, which is the deployment this directory belongs to. 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}`." ) diff --git a/tests/web/test_open_command.py b/tests/web/test_open_command.py index ff75938..49bf8cf 100644 --- a/tests/web/test_open_command.py +++ b/tests/web/test_open_command.py @@ -310,3 +310,45 @@ 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 && ... && keel open` searched `/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 From f437539182c3cf76f767a874edef7b43faa521f9 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 8 Sep 2026 03:49:15 -0400 Subject: [PATCH 2/3] fix(cli): a refusal must not say the process is gone when the process is running Review of this PR's own diff, and the finding is the same class of fault the PR exists to remove: a message that states something false with confidence. Since #759 added the port probe, `live_record` has returned `None` for TWO reasons -- a dead pid, or a live one with nothing answering on the port -- and the refusal asserted the first regardless. Measured against a record whose pid was the running test process: recorded pid: 53702 (this process, alive) Error: ... but its process is gone -- it crashed or was killed. It said that about the very process printing it. A server alive but not yet listening, or bound to a host other than the one recorded, was told it had crashed -- which sends its operator to the wrong investigation entirely, on the surface whose whole job is to stop exactly that. `_refuse` re-checks the pid now and says which failure it is. The live-pid branch names the pid and the address nothing answered on, and points at the log, because that state is a bind failure or a half-started server: a keel server was recorded on port 58030 and its process (27325) is still running, but nothing answers on 127.0.0.1:58030. That is a description of what `com.keel.serve.live` was actually doing on the machine that reported the original bug. `_process_alive` becomes public for the second caller. `_refuse` is typed `NoReturn`, so the unreachable `return` after it is gone and a checker proves what a reader had been assuming. And the not-found message no longer says the path came from "this directory": `state_root` honours `KEEL_HOME` FIRST, so that was false for precisely the reader most likely to have set it. 6,417 passed / 3 skipped, ruff and mypy clean. Four mutants killed, each verified to have applied: always-claim-gone, `NoReturn` reverted, `KEEL_HOME` dropped from the message, and the pid no longer named. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/commands/open_console.py | 34 ++++++++++++++++------- keel/web/runtime.py | 8 ++++-- tests/web/test_open_command.py | 49 ++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 11 deletions(-) diff --git a/keel/commands/open_console.py b/keel/commands/open_console.py index 5fab83b..a08b84d 100644 --- a/keel/commands/open_console.py +++ b/keel/commands/open_console.py @@ -19,6 +19,7 @@ from __future__ import annotations import webbrowser +from typing import NoReturn import click @@ -50,7 +51,6 @@ def open_cmd(port: int, no_browser: bool) -> None: 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") @@ -67,7 +67,7 @@ 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 @@ -86,18 +86,34 @@ def _refuse(port: int) -> None: 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 = int(stale.get("pid", 0) or 0) + 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 " - f"`launchctl kickstart` the agent that runs it).\n\n" + 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" - f" Looked in {searched}, which is the deployment this directory belongs to. 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" + 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, because " diff --git a/keel/web/runtime.py b/keel/web/runtime.py index d3774e3..6979fca 100644 --- a/keel/web/runtime.py +++ b/keel/web/runtime.py @@ -173,9 +173,13 @@ def read_record(port: int) -> dict[str, Any] | None: return parsed -def _process_alive(pid: int) -> bool: +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 @@ -229,7 +233,7 @@ def live_record(port: int) -> dict[str, Any] | None: pid = int(record.get("pid", 0)) except TypeError, ValueError: return None - if not _process_alive(pid): + if not process_alive(pid): 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` diff --git a/tests/web/test_open_command.py b/tests/web/test_open_command.py index 49bf8cf..3695128 100644 --- a/tests/web/test_open_command.py +++ b/tests/web/test_open_command.py @@ -352,3 +352,52 @@ def test_the_help_says_the_deployment_comes_from_the_working_directory() -> None 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 From b0356babacbc0a6bd1941ecada8d17026a207edf Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 8 Sep 2026 17:42:14 -0400 Subject: [PATCH 3/3] fix(web): one place parses the recorded pid, because two is how it broke Re-review of this PR. The previous commit taught the refusal to distinguish a dead process from a live one that is not listening -- and to do it, parsed the pid a SECOND time, bare: ValueError: invalid literal for int() with base 10: 'not-a-number' `live_record` wrapped its conversion in `try`/`except`; `_refuse` repeated it without one. `read_record` admits such a record, because it validates only that a token is present. So a malformed pid tracebacked out of the one code path whose entire job is to fail gracefully -- the exact promise `read_record`'s docstring makes ("`keel open` must not traceback at an operator whose server has just died"). `runtime.recorded_pid` is now the single answer both callers ask for. Fixing the duplicate rather than adding a second guard, because two conversions of one field is what produced this: guarding the new copy would have left the shape that generates the next one. `0` for anything unparseable is the safe reading: `process_alive` refuses zero before signalling -- `os.kill(0, ...)` addresses this process's own group -- so an unreadable pid reports as not running rather than as something. 6,419 passed / 3 skipped, ruff and mypy clean. Three mutants killed: the guard removed, `_refuse` parsing for itself again, and `recorded_pid` always answering zero. The first attempt at the guard-removal mutant DID NOT APPLY -- `ruff format` had rewritten `except (A, B):` into PEP 758's unparenthesised form, so the anchor missed and the suite went green over an unmutated file. Caught by the assertion that the source actually changed, which is the third time that rewrite has voided a patch string in this series. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/commands/open_console.py | 2 +- keel/web/runtime.py | 25 ++++++++++++++++++++----- tests/web/test_open_command.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/keel/commands/open_console.py b/keel/commands/open_console.py index a08b84d..395ebe0 100644 --- a/keel/commands/open_console.py +++ b/keel/commands/open_console.py @@ -93,7 +93,7 @@ def _refuse(port: int) -> NoReturn: # 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 = int(stale.get("pid", 0) or 0) + 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 " diff --git a/keel/web/runtime.py b/keel/web/runtime.py index 6979fca..da4ae4e 100644 --- a/keel/web/runtime.py +++ b/keel/web/runtime.py @@ -173,6 +173,25 @@ def read_record(port: int) -> dict[str, Any] | None: return parsed +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? @@ -229,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` diff --git a/tests/web/test_open_command.py b/tests/web/test_open_command.py index 3695128..c14d064 100644 --- a/tests/web/test_open_command.py +++ b/tests/web/test_open_command.py @@ -8,6 +8,7 @@ from __future__ import annotations +import json import os from pathlib import Path @@ -401,3 +402,32 @@ def test_the_refusal_does_not_claim_the_path_came_from_the_directory_when_it_may 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