From 8cf9e140bdd5e6804e3924ac3b0b138d21f4bb49 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 7 Sep 2026 13:52:58 -0400 Subject: [PATCH 1/2] feat(cli): keel open -- the way back into a console nobody watched start (#756) `keel serve` prints its URL once, with the session token in it. That is enough when a human is watching and useless when nothing is: under launchd the line goes to StandardOutPath, and the route back into your own console becomes grepping a log for a token. Step 4 of #756 -- the four serve plists -- is unusable without this, which is why it comes first. THE TOKEN GOES TO DISK, AND THAT IS BOUNDED BY ONE RULE `security.py` layer 3 rests on the token being ephemeral, and `serve` PRINTS the claim every run: "the token is new every run and is never written to disk". A lockfile makes that sentence false, so it is written under one condition: THE RECORD EXISTS ONLY WHEN THE URL IS NOT BEING SHOWN TO A HUMAN. `sys.stdout.isatty()` decides. Attached to a terminal nothing is written, the posture is byte-for-byte what it was, and the sentence is still printed because it is still true. Detached, a DIFFERENT sentence is printed -- naming the file, its mode, and `keel open` -- because telling an operator their live credential is not on disk while writing it there is the class of false safety assurance `_session_banner` exists to refuse. The argument is not that persisting a token is fine. It is that on the detached path the token is ALREADY in StandardOutPath at the daemon's umask, so the real choice is between that log line and a 0600 file in a 0700 directory that is deleted on shutdown and checked for staleness before it is offered. The second is strictly less exposure, and it is the only one that makes unattended serve usable. `security.py`'s layer 3 now says which of the two runs it is describing. THE FILE IS A HINT, NEVER THE AUTHORITY `forget` covers the clean stop. A SIGKILL or a power cut does not, so `live_record` re-checks the pid with signal 0 before offering anything -- handing over a URL that refuses the browser would read as keel being broken rather than as keel being stopped. `pid <= 0` is refused before the call, because `os.kill(0, 0)` signals the process GROUP and would report a bad record as live. Written through a temporary and renamed, so no reader sees a half-written record. Corrupt JSON reads as absent rather than raising: `keel open` must not traceback at an operator whose server just died. One file per PORT, because #756 counts four deployments and one shared file would make the last server to start the only reachable one. The token is never in the filename -- a listing is readable by anything that can list the directory, even when the file is not. THREE ABSENCES, THREE SENTENCES No server, a server started from a terminal, and a server that was killed look identical from here. "not found" would send an operator hunting for a bug in the two cases where nothing is wrong, so each gets its own message and the interactive one names the terminal that has the URL. 6,352 passed / 3 skipped, ruff and mypy clean. Four mutants killed: the record written while a human watches, the record never forgotten, the liveness check skipped, and the file at 0644. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/cli.py | 4 + keel/commands/open_console.py | 86 +++++++++++++ keel/web/runtime.py | 210 +++++++++++++++++++++++++++++++ keel/web/security.py | 13 +- keel/web/server.py | 22 +++- tests/web/test_open_command.py | 168 +++++++++++++++++++++++++ tests/web/test_runtime_record.py | 128 +++++++++++++++++++ 7 files changed, 626 insertions(+), 5 deletions(-) create mode 100644 keel/commands/open_console.py create mode 100644 keel/web/runtime.py create mode 100644 tests/web/test_open_command.py create mode 100644 tests/web/test_runtime_record.py diff --git a/keel/cli.py b/keel/cli.py index 2f2ddc4..5add21d 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -161,6 +161,7 @@ from keel.commands.journal import journal_group from keel.commands.mcp import mcp_cmd from keel.commands.monitor import run_monitor +from keel.commands.open_console import open_cmd from keel.commands.orders import orders_cmd from keel.commands.pnl import build_pnl_report, render_pnl_report from keel.commands.posture import posture_group @@ -1446,6 +1447,9 @@ def simulate( # dashboard's autonomy action and the console's three gated actions went. Each of those four # mirrored a CLI command that is still here. cli.add_command(serve_cmd) +# #756: the way back to a console nobody watched start. See `keel/web/runtime.py` for why a +# detached server may leave its token on disk and an interactive one may not. +cli.add_command(open_cmd) # -- mcp (the read-only research-assistant surface over stdio) ----------------------------------- diff --git a/keel/commands/open_console.py b/keel/commands/open_console.py new file mode 100644 index 0000000..539f2ab --- /dev/null +++ b/keel/commands/open_console.py @@ -0,0 +1,86 @@ +"""`keel open` -- reach a console that is already running (#756). + +`keel serve` prints its URL once, at startup, with the session token in it. That is enough when a +human is watching the terminal and useless when nothing is: under `launchd` the line goes to +`StandardOutPath`, and the way back into your own console becomes `grep`-ing a log for a token. + +So a detached `serve` records how to reach itself (`keel/web/runtime.py`, which carries the +argument for why that is acceptable and how it is bounded), and this reads that record back. + +**It mints nothing and it extends nothing.** The token it prints is the one the running server +already minted; stopping keel still revokes it, and this command has no way to bring it back. + +**It refuses rather than guesses.** Three different absences look identical from here -- no server, +a server started from a terminal, and a server that was killed -- and each gets its own sentence, +because "not found" would send an operator hunting for a bug in the two cases where nothing is +wrong. +""" + +from __future__ import annotations + +import webbrowser + +import click + +from keel.commands.serve import DEFAULT_PORT +from keel.web import runtime + + +@click.command("open") +@click.option( + "--port", + default=DEFAULT_PORT, + show_default=True, + type=int, + help="Which console. One `keel serve` process per port; see `keel serve --port`.", +) +@click.option( + "--no-browser", + is_flag=True, + default=False, + 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.""" + 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") + click.echo("This address carries the running server's session token. Stopping keel revokes it.") + + if no_browser: + return + try: + # Best-effort, exactly as `keel serve` treats its own launch: the URL is already printed, + # so a headless machine, a broken BROWSER variable or a sandbox with no launcher costs the + # operator nothing they cannot recover by pasting. + webbrowser.open(url) + except Exception as exc: # noqa: BLE001 -- any launcher failure is survivable here + click.echo(f"(could not launch a browser: {exc})") + + +def _refuse(port: int) -> None: + """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. + """ + 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)." + ) + 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` " + "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" + f" If none is running, start one: `keel serve --port {port}`." + ) diff --git a/keel/web/runtime.py b/keel/web/runtime.py new file mode 100644 index 0000000..544dbb7 --- /dev/null +++ b/keel/web/runtime.py @@ -0,0 +1,210 @@ +"""Where a running `keel serve` says how to reach it, so `keel open` can answer (#756). + +── WHY THIS EXISTS AT ALL, GIVEN WHAT `security.py` SAYS ────────────────────────────────────── + +Layer 3 of `keel/web/security.py` rests on the session token being ephemeral: *"the token is +minted per process and never written to disk, so closing the server destroys it and every +outstanding cookie becomes a string that authenticates nothing."* `serve` prints that sentence to +the operator on every run. + +This module writes the token to disk, so it is bounded by one rule and the rule is the whole +design: + + THE RECORD EXISTS ONLY WHEN THE URL IS NOT BEING SHOWN TO A HUMAN. + +`sys.stdout.isatty()` decides. Attached to a terminal, nothing here runs and the posture is +byte-for-byte what it was; `serve` still prints the sentence, and the sentence is still true. + +Detached -- `launchd`, a pipe, a container -- the token is going to a file ANYWAY, because that is +what `StandardOutPath` is. An operator's only route back into their own console then is to `grep` +a log for a URL. So the choice at that point is not "token on disk or not"; it is "token in a log +file at the daemon's umask, or token in a `0600` file in a `0700` directory that is deleted on +shutdown and whose staleness is checked before it is offered". The second is strictly less +exposure than the first, and it is the only one that makes `keel serve` usable unattended. + +── WHAT THIS IS NOT ─────────────────────────────────────────────────────────────────────────── + +**Not an authority on whether a server is running.** A `SIGKILL` or a power cut leaves the file +behind, so `live_record` re-checks the pid before offering anything. The file is a hint. + +**Not a way to reissue or extend a token.** Nothing here mints; it records what `serve` already +minted, and `forget` drops it. Stopping keel still revokes every outstanding cookie, which is +the operator's revocation gesture and is unchanged. + +**Not a lock.** It does not arbitrate who may bind a port -- the OS already refuses a second +bind, and a lock file that could disagree with the kernel would be a second answer to a question +that already has one. +""" + +from __future__ import annotations + +import json +import os +import sys +import time +from pathlib import Path +from typing import Any + +from keel_core.paths import state_root + +#: The directory holding one record per serving port. Under the deployment root, beside the +#: database rather than in a global `/tmp`: two deployments on one machine are the ordinary case +#: (#756 counts four), and their consoles must not read each other's records. +RUN_DIR_NAME = "run" + +#: `0700` on the directory and `0600` on the file. Group and world get nothing: this holds a live +#: bearer token, and on a shared machine a readable one would be worse than the log line it +#: replaces. Both are asserted by `tests/web/test_runtime_record.py`. +DIR_MODE = 0o700 +FILE_MODE = 0o600 + + +def run_dir(*, create: bool = False) -> Path: + directory = state_root() / RUN_DIR_NAME + if create: + directory.mkdir(parents=True, exist_ok=True) + # Set explicitly rather than trusting the umask: `mkdir`'s mode is masked, and a + # deployment running under a permissive umask would otherwise get a group-readable + # directory holding session tokens. + directory.chmod(DIR_MODE) + return directory + + +def record_path(port: int) -> Path: + """One file per port. The PORT names it and the token lives inside. + + Directory listings leak names, and a filename is readable by anything that can list the + directory even when the file itself is not. Pinned by + `test_the_token_is_never_in_the_filename`. + """ + return run_dir() / f"serve-{int(port)}.json" + + +def record_serving(*, host: str, port: int, token: str, interactive: bool) -> Path | None: + """Record how to reach this server, unless a human is watching stdout. + + `interactive` is the caller's `sys.stdout.isatty()` -- passed in rather than read here so the + decision is visible at the call site in `serve`, where the operator-facing sentence about it + is also printed, and so a test can exercise both sides without touching the process's own + streams. + + Returns the path written, or `None` when the rule above says to write nothing. + """ + if interactive: + return None + directory = run_dir(create=True) + path = directory / f"serve-{int(port)}.json" + # Written through a per-port temporary file and then renamed, so a reader can never see a + # half-written record: `os.replace` is atomic within a directory. The temporary carries the + # same `0600`, because it holds the same token for the moment it exists. + staging = directory / f".serve-{int(port)}.json.tmp" + staging.write_text( + json.dumps( + { + "pid": os.getpid(), + "host": host, + "port": int(port), + "token": token, + "started_ts": int(time.time()), + } + ) + ) + staging.chmod(FILE_MODE) + os.replace(staging, path) + return path + + +def read_record(port: int) -> dict[str, Any] | None: + """The record for `port`, or `None` if there is none or it cannot be read. + + Deliberately does NOT check liveness -- `live_record` does, and keeping them apart means a + caller diagnosing a stale file can still see it. A corrupt or truncated file (a crash + mid-write) reads as absent rather than raising: `keel open` must not traceback at an operator + whose server has just died. + """ + path = record_path(port) + try: + raw = path.read_text() + except OSError: + return None + try: + parsed = json.loads(raw) + except ValueError: + return None + if not isinstance(parsed, dict) or not parsed.get("token"): + return None + return parsed + + +def _process_alive(pid: int) -> bool: + """Is `pid` a process this user could signal? + + `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 + stale record as live. + """ + if pid <= 0: + return False + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + # It exists and belongs to someone else. Not ours to open, and not ours to claim is gone. + return True + except OSError: + return False + return True + + +def live_record(port: int) -> dict[str, Any] | None: + """The record for `port`, but only if the process that wrote it is still around. + + `forget` covers the clean exit. This covers every other kind: a `SIGKILL`, a panic, a power + cut. Without it `keel open` would hand an operator a URL that refuses them, which reads as + keel being broken rather than as keel being stopped. + """ + record = read_record(port) + if record is None: + return None + try: + pid = int(record.get("pid", 0)) + except TypeError, ValueError: + return None + return record if _process_alive(pid) else None + + +def forget(port: int) -> None: + """Drop the record. Called from `serve`'s `finally`, so a clean stop leaves nothing behind.""" + try: + record_path(port).unlink() + except OSError: + # Already gone, or a directory that never existed because the run was interactive. Both + # are the desired end state, and neither is worth failing a shutdown over. + pass + + +def url_for(record: dict[str, Any]) -> str: + """The address to open, rebuilt from a record. + + The bracketing rule for IPv6 is `ServeConfig.url`'s, restated because this side has no + `ServeConfig` to ask -- `keel open` reads a file written by a process that has since become + unreachable. `test_an_ipv6_host_is_bracketed_like_serve_prints_it` holds the two spellings + to the same output. + """ + host = str(record.get("host", "")) + bracketed = f"[{host}]" if ":" in host else host + return f"http://{bracketed}:{int(record.get('port', 0))}/?token={record.get('token', '')}" + + +def stdout_is_interactive() -> bool: + """Whether a human is watching this process's stdout. + + Wrapped rather than called inline so `serve` reads as the rule it implements, and because a + detached stream can raise on `isatty` rather than answering. + """ + try: + return bool(sys.stdout.isatty()) + except AttributeError, ValueError: + return False diff --git a/keel/web/security.py b/keel/web/security.py index cf6c1b1..d4047e4 100644 --- a/keel/web/security.py +++ b/keel/web/security.py @@ -21,9 +21,16 @@ user is already viewing. `keel serve` mints it per run, prints it in the URL, and the browser exchanges it for a `SameSite=Strict` cookie on first load. `Strict` (not `Lax`) is deliberate: `Lax` attaches the cookie to top-level navigations, so a link on a hostile page would arrive - authenticated. Nothing is persisted **on this side of the wire** -- the token is minted per - process and never written to disk, so closing the server destroys it and every outstanding - cookie becomes a string that authenticates nothing. Since #634 the cookie carries a `Max-Age` + authenticated. The token is minted per process, so closing the server destroys it and every + outstanding cookie becomes a string that authenticates nothing. + + **Whether it is written to disk depends on who is watching, and #756 is why.** Attached to a + terminal, nothing is persisted on this side of the wire -- unchanged. Detached (`launchd`, a + pipe), `keel/web/runtime.py` records the address in a `0600` file so `keel open` can hand it + back; read its module docstring before touching that, because the argument is not "it is fine + to persist a token" but "on that path the token is already in `StandardOutPath` at the daemon's + umask, and a mode-`0600` file deleted on shutdown is strictly less exposure than the log line + that would otherwise be the only way in". Since #634 the cookie carries a `Max-Age` so the BROWSER stops throwing away a token that is still valid; `SESSION_COOKIE_MAX_AGE_SECONDS` carries the whole argument for why that extends convenience and not authority. diff --git a/keel/web/server.py b/keel/web/server.py index 9169ce9..d7ad499 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -51,7 +51,7 @@ from typing import Any from urllib.parse import parse_qs, urlsplit -from keel.web import api, events, staticfiles +from keel.web import api, events, runtime, staticfiles from keel.web.security import ( CSRF_HEADER, REMOTE_SESSION_MAX_AGE_SECONDS, @@ -988,7 +988,21 @@ def serve(cfg: ServeConfig, *, echo: Callable[[str], None] = print) -> int: # way to sign out. Said here rather than only in a docstring, because the operator who needs # to revoke is the operator reading this terminal, and the gesture is the one they already # have: stopping keel invalidates the token, so every browser holding it is out. - echo("Stopping keel revokes it: the token is new every run and is never written to disk.") + # #756. The sentence an operator is owed depends on which of these two runs this is, and + # printing the wrong one would be a false safety assurance about a live credential -- the + # class of thing `payload._session_banner` refuses to do about mode. + interactive = runtime.stdout_is_interactive() + recorded = runtime.record_serving( + host=running.host, port=running.port, token=running.token, interactive=interactive + ) + if recorded is None: + echo("Stopping keel revokes it: the token is new every run and is never written to disk.") + else: + echo("Stopping keel revokes it: the token is new every run.") + echo( + f"Nothing is reading this output, so the address is also in {recorded} (0600) -- " + f"run `keel open --port {running.port}` to get it back. It is deleted on shutdown." + ) try: server.serve_forever() @@ -996,6 +1010,10 @@ def serve(cfg: ServeConfig, *, echo: Callable[[str], None] = print) -> int: echo("") echo("stopped.") finally: + # Before `server_close`, so a record never outlives the port it names by more than the + # instant between these two lines. A crash skips this entirely, which is what + # `runtime.live_record`'s pid check is for. + runtime.forget(running.port) server.server_close() return 0 diff --git a/tests/web/test_open_command.py b/tests/web/test_open_command.py new file mode 100644 index 0000000..20b49c7 --- /dev/null +++ b/tests/web/test_open_command.py @@ -0,0 +1,168 @@ +"""`keel open`, and what `keel serve` has to leave behind for it to work (#756). + +Under `launchd` nobody reads stdout, so the URL carrying the session token lands in a log file and +an operator's only way back into their own console is `grep`. These tests hold both halves of the +fix: that `serve` records how to reach itself when — and only when — no human is watching, and +that `open` refuses clearly in every case where it cannot help. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest +from click.testing import CliRunner + +from keel.cli import cli +from keel.web import runtime +from keel.web import server as web_server +from keel.web.security import new_session_token + + +@pytest.fixture() +def home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + monkeypatch.setenv("KEEL_HOME", str(tmp_path)) + return tmp_path + + +class _StubServer: + def __init__(self, address: tuple[str, int]) -> None: + self.server_address = address + self.RequestHandlerClass = type("H", (), {"cfg": None}) + self.closed = False + + def serve_forever(self) -> None: + raise KeyboardInterrupt + + def server_close(self) -> None: + self.closed = True + + +def _serve(monkeypatch: pytest.MonkeyPatch, home: Path, *, interactive: bool) -> list[str]: + stub = _StubServer(("127.0.0.1", 8765)) + monkeypatch.setattr(web_server, "ensure_schema", lambda _path: None) + monkeypatch.setattr(web_server, "build_server", lambda _cfg: stub) + monkeypatch.setattr(runtime, "stdout_is_interactive", lambda: interactive) + lines: list[str] = [] + cfg = web_server.ServeConfig( + host="127.0.0.1", + port=8765, + token=new_session_token(), + db_path=str(home / "keel.db"), + config_path=str(home / "config.yaml"), + ) + assert web_server.serve(cfg, echo=lines.append) == 0 + return lines + + +# -- what serve leaves behind ------------------------------------------------------------------- + + +def test_an_interactive_serve_writes_nothing_and_says_the_token_never_lands( + monkeypatch: pytest.MonkeyPatch, home: Path +) -> None: + """The posture `security.py` describes, unchanged, and the sentence still true.""" + lines = _serve(monkeypatch, home, interactive=True) + assert not (home / "run").exists() + assert any("never written to disk" in line for line in lines), lines + + +def test_a_detached_serve_records_itself_and_says_so( + monkeypatch: pytest.MonkeyPatch, home: Path +) -> None: + """And it must NOT print the never-written sentence, which would then be false. + + The operator is owed the difference: on this path the token is on disk, and a line claiming + otherwise is exactly the sort of false safety assurance `_session_banner` exists to refuse. + """ + lines = _serve(monkeypatch, home, interactive=False) + assert not any("never written to disk" in line for line in lines), lines + assert any("keel open" in line for line in lines), lines + + +def test_a_clean_stop_removes_the_record(monkeypatch: pytest.MonkeyPatch, home: Path) -> None: + """`serve`'s stub raises `KeyboardInterrupt` from `serve_forever`, which is the Ctrl-C path.""" + _serve(monkeypatch, home, interactive=False) + assert runtime.read_record(8765) is None, "the record outlived the server that wrote it" + + +# -- the command -------------------------------------------------------------------------------- + + +def test_open_is_registered_and_documents_itself() -> None: + result = CliRunner().invoke(cli, ["open", "--help"]) + assert result.exit_code == 0 + assert "--port" in result.output + + +def test_open_prints_the_url_when_a_server_is_recorded(home: Path) -> None: + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + result = CliRunner().invoke(cli, ["open", "--port", "8765", "--no-browser"]) + assert result.exit_code == 0, result.output + assert "http://127.0.0.1:8765/?token=tok" in result.output + + +def test_open_refuses_when_nothing_is_recorded(home: Path) -> None: + """Non-zero, because a script that pipes this into a browser must be able to tell.""" + result = CliRunner().invoke(cli, ["open", "--port", "8765", "--no-browser"]) + assert result.exit_code != 0 + assert "8765" in result.output + + +def test_open_explains_the_interactive_case_rather_than_just_failing(home: Path) -> None: + """The likeliest confusion this command will cause: a server IS running, started from a + terminal, and deliberately left no record. "not found" would send the operator looking for a + bug; naming the reason sends them to the terminal that has the URL.""" + result = CliRunner().invoke(cli, ["open", "--port", "8765", "--no-browser"]) + assert "terminal" in result.output.lower(), result.output + + +def test_open_does_not_offer_a_url_for_a_process_that_has_gone(home: Path) -> None: + """A `SIGKILL` leaves the file. Handing over a URL that refuses the browser would read as keel + being broken rather than as keel being stopped.""" + # A value no English sentence contains. `"tok"` was the first choice and it matched the word + # "token" in the refusal itself -- a substring assertion failing on prose, which is the same + # trap in the other direction from a substring assertion PASSING on prose. + secret = "Z9-stale-secret-Z9" + runtime.record_serving(host="127.0.0.1", port=8765, token=secret, interactive=False) + path = runtime.record_path(8765) + path.write_text(path.read_text().replace('"pid": ' + str(os.getpid()), '"pid": 0')) + result = CliRunner().invoke(cli, ["open", "--port", "8765", "--no-browser"]) + assert result.exit_code != 0 + assert secret not in result.output, "a stale token was printed anyway" + + +def test_open_launches_a_browser_by_default_and_can_be_told_not_to( + home: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Symmetric with `keel serve --no-open`: the URL is printed either way, so a machine with no + launcher loses nothing.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + opened: list[str] = [] + import keel.commands.open_console as open_mod + + monkeypatch.setattr(open_mod.webbrowser, "open", lambda url: opened.append(url) or True) + + CliRunner().invoke(cli, ["open", "--port", "8765", "--no-browser"]) + assert opened == [] + + CliRunner().invoke(cli, ["open", "--port", "8765"]) + assert opened == ["http://127.0.0.1:8765/?token=tok"] + + +def test_a_browser_that_will_not_launch_does_not_fail_the_command( + home: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`serve` treats the launch as best-effort for the same reason: the URL is already printed, + and typing it in is a complete fallback.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + import keel.commands.open_console as open_mod + + def _boom(_url: str) -> bool: + raise RuntimeError("no browser here") + + monkeypatch.setattr(open_mod.webbrowser, "open", _boom) + result = CliRunner().invoke(cli, ["open", "--port", "8765"]) + assert result.exit_code == 0, result.output + assert "http://127.0.0.1:8765/?token=tok" in result.output diff --git a/tests/web/test_runtime_record.py b/tests/web/test_runtime_record.py new file mode 100644 index 0000000..7482c00 --- /dev/null +++ b/tests/web/test_runtime_record.py @@ -0,0 +1,128 @@ +"""The serve runtime record: a live session token on disk, and the rule that bounds it (#756). + +`keel serve` mints a session token per process and prints it in a URL. Under `launchd` nobody +reads stdout, so that URL -- token and all -- lands in `StandardOutPath`, a log file at whatever +umask the daemon runs under. An operator's only way back into their own console becomes `grep`. + +`keel open` fixes that, and the only way it can is by the server leaving the token somewhere +readable. That is a real weakening of layer 3 in `keel/web/security.py` ("the token is minted per +process and never written to disk"), so it is bounded by one rule, which these tests are about: + + THE RECORD EXISTS ONLY WHEN THE URL IS NOT BEING SHOWN TO A HUMAN. + +`sys.stdout.isatty()` is the test. Attached to a terminal, the operator already has the URL and +nothing is written -- today's posture, byte for byte. Not attached, the token is already going +somewhere persistent (a log, a pipe), and a `0600` file in a `0700` directory that is deleted on +shutdown is strictly *less* exposure than the log line that would otherwise be the only way in. +""" + +from __future__ import annotations + +import json +import os +import stat +from pathlib import Path + +import pytest + +from keel.web import runtime + + +@pytest.fixture() +def home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + monkeypatch.setenv("KEEL_HOME", str(tmp_path)) + return tmp_path + + +def test_nothing_is_written_when_stdout_is_a_terminal(home: Path) -> None: + """The whole bound on this feature. An interactive `keel serve` keeps the posture its own + output claims -- and the sentence it prints stays true.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=True) + assert runtime.read_record(8765) is None + assert not (home / "run").exists(), "an interactive run created the runtime directory" + + +def test_the_record_is_written_when_nobody_is_reading_stdout(home: Path) -> None: + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + record = runtime.read_record(8765) + assert record is not None + assert record["token"] == "tok" + assert record["port"] == 8765 + assert record["pid"] == os.getpid() + + +def test_the_file_is_unreadable_by_anyone_else(home: Path) -> None: + """`0600` in a `0700` directory. It holds a live bearer token; group or world read would put + it within reach of every account on a shared machine, which is worse than the log line this + exists to replace.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + path = runtime.record_path(8765) + assert stat.S_IMODE(path.stat().st_mode) == 0o600, oct(path.stat().st_mode) + assert stat.S_IMODE(path.parent.stat().st_mode) == 0o700, oct(path.parent.stat().st_mode) + + +def test_the_record_is_removed_on_shutdown(home: Path) -> None: + """A token that outlives its process authenticates nothing, but it still READS as a way in, + and an operator following a stale record would be told to open a URL that refuses them.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + runtime.forget(8765) + assert runtime.read_record(8765) is None + + +def test_a_record_whose_process_is_gone_is_not_offered(home: Path) -> None: + """Removal on shutdown covers the clean exit. A `SIGKILL`, a panic or a power cut leaves the + file behind, so liveness is checked on the way OUT as well -- the file is a hint, never the + authority on whether a server is running.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + path = runtime.record_path(8765) + stale = json.loads(path.read_text()) + # A pid this process can prove is not serving: its own parent's parent is not `keel serve`, + # and pid 0 is never a real process to signal. + stale["pid"] = 0 + path.write_text(json.dumps(stale)) + assert runtime.live_record(8765) is None + assert runtime.read_record(8765) is not None, "read_record must not silently drop a stale file" + + +def test_a_corrupt_record_is_ignored_rather_than_raising(home: Path) -> None: + """A truncated write (a crash mid-flush) must not make `keel open` traceback at an operator + who is already having a bad day.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + runtime.record_path(8765).write_text("{not json") + assert runtime.read_record(8765) is None + + +def test_the_token_is_never_in_the_filename(home: Path) -> None: + """Directory listings leak names. The port identifies the record; the secret is inside.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="s3cret-token", interactive=False) + assert "s3cret-token" not in str(runtime.record_path(8765)) + assert "s3cret-token" not in "".join(p.name for p in runtime.record_path(8765).parent.iterdir()) + + +def test_records_are_per_port_so_several_deployments_coexist(home: Path) -> None: + """The point of #756: four deployments, four consoles, four ports. One shared file would make + the last server to start the only one reachable.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="live", interactive=False) + runtime.record_serving(host="127.0.0.1", port=8766, token="paper", interactive=False) + live = runtime.read_record(8765) + paper = runtime.read_record(8766) + assert live is not None and paper is not None + assert live["token"] == "live" + assert paper["token"] == "paper" + assert runtime.record_path(8765) != runtime.record_path(8766) + + +def test_the_url_is_rebuilt_from_the_record_and_carries_the_token(home: Path) -> None: + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + record = runtime.read_record(8765) + assert record is not None + assert runtime.url_for(record) == "http://127.0.0.1:8765/?token=tok" + + +def test_an_ipv6_host_is_bracketed_like_serve_prints_it(home: Path) -> None: + """`ServeConfig.url` brackets it; a second spelling of one rule is a second chance to get it + wrong, so this asserts the same shape.""" + runtime.record_serving(host="::1", port=8765, token="tok", interactive=False) + record = runtime.read_record(8765) + assert record is not None + assert runtime.url_for(record) == "http://[::1]:8765/?token=tok" From 938aed7f8ec08332cfbf1980a6007751fdfaccb2 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 7 Sep 2026 17:27:43 -0400 Subject: [PATCH 2/2] fix(web): create the token file at 0600, probe the port, and never make a deployment Review of this PR's own diff. Three findings, one of them the line CodeQL is pointing at. THE TOKEN WAS BORN WORLD-READABLE `Path.write_text` creates at the process umask -- measured 0644 under the usual 022 -- and the `chmod(0600)` on the next line closed the window after the fact. The 0700 directory meant no other account could traverse in, so it was never exploitable; it was also an incidental mitigation for the one file whose entire purpose is holding a secret. `os.open(..., O_CREAT | O_EXCL | O_WRONLY, 0600)` now creates it at the mode it needs, and `O_EXCL` means a symlink planted at the staging name cannot redirect the write either. The test works by REMOVING THE SAFETY NET: with `chmod` neutered, a file merely corrected afterwards shows its umask mode and one created correctly still shows 0600. Asserting the final mode alone -- which the existing test does -- passes either way. A PID CHECK IS NOT LIVENESS keel dies, the OS hands its pid to anything else, and the record reads as live: `keel open` prints a token the server no longer honours, the browser gets a 403, and that is precisely the "keel is broken rather than stopped" confusion the check exists to prevent. `live_record` now also requires something to ANSWER on the recorded port. Both, because neither is sufficient: a listening port could belong to another program and a live pid could be a recycled one. A connect, not a request -- this must not send the token anywhere. The original spec asked for a process active ON THE DESIGNATED PORT. Half of it shipped. SERVING BROUGHT A DEPLOYMENT INTO EXISTENCE `mkdir(parents=True)` reached straight past `state_root`'s own contract: "it never creates a deployment folder, because a deployment folder that does not exist is not one this function chose". `server.ensure_schema` refuses the identical hazard one directory over. `parents=False` now, and no deployment means no record -- the operator on that path is being shown the setup page and has no console to reopen yet. AND THREE TESTS ONLY PASSED BECAUSE OF THIS MACHINE Adding the port probe turned `port=8765` from an arbitrary number into a claim about the host running the suite. Three tests expecting `keel open` to SUCCEED went green here only because a real `keel serve` happened to be listening on 8765, and would have failed in CI. They bind a real listener now; the refusal tests take a port proven closed. Found because the first cut of the recycled-pid test failed for the opposite reason -- it asserted nothing was on 8765, and something was. (The backlog of that listener is 8, not 1: nothing accepts, `live_record` connects once per call, and the test calls it twice.) 6,356 passed / 3 skipped, ruff and mypy clean. Three mutants killed: write_text plus chmod, the port probe removed, and parents=True restored. CodeQL's clear-text-storage alert on this file is not addressed by any of the above and is not a false positive: the token IS stored as clear text, by design, under the isatty rule the module docstring argues for. Dismissing or rejecting that is a repository-security decision, not a code change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/web/runtime.py | 97 ++++++++++++++++++++----- tests/web/test_open_command.py | 118 ++++++++++++++++++++++++++----- tests/web/test_runtime_record.py | 41 +++++++++++ 3 files changed, 221 insertions(+), 35 deletions(-) diff --git a/keel/web/runtime.py b/keel/web/runtime.py index 544dbb7..d3774e3 100644 --- a/keel/web/runtime.py +++ b/keel/web/runtime.py @@ -40,6 +40,7 @@ import json import os +import socket import sys import time from pathlib import Path @@ -58,11 +59,31 @@ DIR_MODE = 0o700 FILE_MODE = 0o600 +#: How long to wait for the recorded port to answer. Loopback, so a healthy server answers +#: immediately; anything slower is a machine in trouble worth reporting rather than waiting on. +PROBE_TIMEOUT_SECONDS = 0.5 -def run_dir(*, create: bool = False) -> Path: - directory = state_root() / RUN_DIR_NAME + +def run_dir(*, create: bool = False) -> Path | None: + """The run directory, or `None` when there is no deployment to put one in. + + `parents=False` deliberately (#759 review). `state_root`'s own contract is that "it never + creates a deployment folder, because a deployment folder that does not exist is not one this + function chose", and `mkdir(parents=True)` reached straight past it -- serving on a machine + with no deployment brought one into existence as a side effect. `server.ensure_schema` refuses + the identical hazard one directory over ("a read-only view would bring a deployment into + existence merely by being started"), and a first-run `keel serve` with no deployment is a + supported state, not an error. + + So: record into a deployment that exists, and record nothing into one that does not. The + operator on that path is being shown the setup page and has no console to reopen yet. + """ + root = state_root() + directory = root / RUN_DIR_NAME if create: - directory.mkdir(parents=True, exist_ok=True) + if not root.is_dir(): + return None + directory.mkdir(parents=False, exist_ok=True) # Set explicitly rather than trusting the umask: `mkdir`'s mode is masked, and a # deployment running under a permissive umask would otherwise get a group-readable # directory holding session tokens. @@ -77,7 +98,9 @@ def record_path(port: int) -> Path: directory even when the file itself is not. Pinned by `test_the_token_is_never_in_the_filename`. """ - return run_dir() / f"serve-{int(port)}.json" + directory = run_dir() + assert directory is not None # `create=False` always answers a path + return directory / f"serve-{int(port)}.json" def record_serving(*, host: str, port: int, token: str, interactive: bool) -> Path | None: @@ -93,23 +116,37 @@ def record_serving(*, host: str, port: int, token: str, interactive: bool) -> Pa if interactive: return None directory = run_dir(create=True) + if directory is None: + return None path = directory / f"serve-{int(port)}.json" # Written through a per-port temporary file and then renamed, so a reader can never see a # half-written record: `os.replace` is atomic within a directory. The temporary carries the # same `0600`, because it holds the same token for the moment it exists. staging = directory / f".serve-{int(port)}.json.tmp" - staging.write_text( - json.dumps( - { - "pid": os.getpid(), - "host": host, - "port": int(port), - "token": token, - "started_ts": int(time.time()), - } - ) + body = json.dumps( + { + "pid": os.getpid(), + "host": host, + "port": int(port), + "token": token, + "started_ts": int(time.time()), + } ) - staging.chmod(FILE_MODE) + # CREATED at `0600`, not corrected to it (#759 review). `Path.write_text` creates at the + # process umask -- measured `0644` under the usual `022` -- and the `chmod` that followed left + # the token world-readable for the window between the two calls. The `0700` directory meant no + # other account could traverse in, so it was never exploitable; it was also an incidental + # mitigation for the one file whose entire purpose is holding a secret, and the fix is one + # argument to `os.open`. `O_EXCL` refuses a pre-existing path, so a symlink planted at the + # staging name cannot redirect the write either. + descriptor = os.open(staging, os.O_CREAT | os.O_EXCL | os.O_WRONLY, FILE_MODE) + try: + with os.fdopen(descriptor, "w") as handle: + handle.write(body) + except BaseException: + # A half-written staging file must not be left behind to collide with the next `O_EXCL`. + staging.unlink(missing_ok=True) + raise os.replace(staging, path) return path @@ -158,6 +195,26 @@ def _process_alive(pid: int) -> bool: return True +def _port_answers(host: str, port: int) -> bool: + """Can a loopback TCP connection be made to `host:port` right now? + + A connect, not a request: this must not send the token anywhere, and "something is bound" is + the whole question. `create_connection` resolves the family, so an IPv6 record needs no special + case -- and it takes the UNBRACKETED host, unlike `url_for`, because brackets are a URL + spelling rather than part of an address. + + A short timeout on purpose. This runs against loopback, where a healthy answer is immediate; + anything slower is a machine in trouble, and `keel open` should say so rather than hang. + """ + if not host: + return False + try: + with socket.create_connection((host, int(port)), timeout=PROBE_TIMEOUT_SECONDS): + return True + except OSError: + return False + + def live_record(port: int) -> dict[str, Any] | None: """The record for `port`, but only if the process that wrote it is still around. @@ -172,7 +229,15 @@ def live_record(port: int) -> dict[str, Any] | None: pid = int(record.get("pid", 0)) except TypeError, ValueError: return None - return record if _process_alive(pid) else None + 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` + # prints a token the server no longer honours and the browser gets a 403, which is the "keel is + # broken rather than stopped" confusion this check exists to prevent. Both checks, because + # neither is sufficient alone: a listening port could belong to another program, and a live pid + # could be a recycled one. + return record if _port_answers(str(record.get("host", "")), port) else None def forget(port: int) -> None: diff --git a/tests/web/test_open_command.py b/tests/web/test_open_command.py index 20b49c7..af3b00f 100644 --- a/tests/web/test_open_command.py +++ b/tests/web/test_open_command.py @@ -26,6 +26,29 @@ def home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: return tmp_path +@pytest.fixture() +def listening() -> object: + """A port with something really bound to it, yielded as an int. + + `live_record` now probes the port, so every test that expects `keel open` to SUCCEED needs a + real listener. Three of them used a bare 8765 and passed on the author's machine only because + a real `keel serve` happened to be running there -- they would have failed in CI, which is the + same class of environment-dependence as a fixture that reaches the network. + + Backlog room for several probes: nothing here ever `accept()`s, and a backlog of 1 refuses the + second connection. + """ + import socket + + sock = socket.socket() + sock.bind(("127.0.0.1", 0)) + sock.listen(8) + try: + yield int(sock.getsockname()[1]) + finally: + sock.close() + + class _StubServer: def __init__(self, address: tuple[str, int]) -> None: self.server_address = address @@ -96,25 +119,26 @@ def test_open_is_registered_and_documents_itself() -> None: assert "--port" in result.output -def test_open_prints_the_url_when_a_server_is_recorded(home: Path) -> None: - runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) - result = CliRunner().invoke(cli, ["open", "--port", "8765", "--no-browser"]) +def test_open_prints_the_url_when_a_server_is_recorded(home: Path, listening: int) -> None: + runtime.record_serving(host="127.0.0.1", port=listening, token="tok", interactive=False) + result = CliRunner().invoke(cli, ["open", "--port", str(listening), "--no-browser"]) assert result.exit_code == 0, result.output - assert "http://127.0.0.1:8765/?token=tok" in result.output + assert f"http://127.0.0.1:{listening}/?token=tok" in result.output def test_open_refuses_when_nothing_is_recorded(home: Path) -> None: """Non-zero, because a script that pipes this into a browser must be able to tell.""" - result = CliRunner().invoke(cli, ["open", "--port", "8765", "--no-browser"]) + port = _a_closed_port() + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) assert result.exit_code != 0 - assert "8765" in result.output + assert str(port) in result.output def test_open_explains_the_interactive_case_rather_than_just_failing(home: Path) -> None: """The likeliest confusion this command will cause: a server IS running, started from a terminal, and deliberately left no record. "not found" would send the operator looking for a bug; naming the reason sends them to the terminal that has the URL.""" - result = CliRunner().invoke(cli, ["open", "--port", "8765", "--no-browser"]) + result = CliRunner().invoke(cli, ["open", "--port", str(_a_closed_port()), "--no-browser"]) assert "terminal" in result.output.lower(), result.output @@ -125,44 +149,100 @@ def test_open_does_not_offer_a_url_for_a_process_that_has_gone(home: Path) -> No # "token" in the refusal itself -- a substring assertion failing on prose, which is the same # trap in the other direction from a substring assertion PASSING on prose. secret = "Z9-stale-secret-Z9" - runtime.record_serving(host="127.0.0.1", port=8765, token=secret, interactive=False) - path = runtime.record_path(8765) + port = _a_closed_port() + runtime.record_serving(host="127.0.0.1", port=port, token=secret, 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", "8765", "--no-browser"]) + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) assert result.exit_code != 0 assert secret not in result.output, "a stale token was printed anyway" def test_open_launches_a_browser_by_default_and_can_be_told_not_to( - home: Path, monkeypatch: pytest.MonkeyPatch + home: Path, monkeypatch: pytest.MonkeyPatch, listening: int ) -> None: """Symmetric with `keel serve --no-open`: the URL is printed either way, so a machine with no launcher loses nothing.""" - runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + runtime.record_serving(host="127.0.0.1", port=listening, token="tok", interactive=False) opened: list[str] = [] import keel.commands.open_console as open_mod monkeypatch.setattr(open_mod.webbrowser, "open", lambda url: opened.append(url) or True) - CliRunner().invoke(cli, ["open", "--port", "8765", "--no-browser"]) + CliRunner().invoke(cli, ["open", "--port", str(listening), "--no-browser"]) assert opened == [] - CliRunner().invoke(cli, ["open", "--port", "8765"]) - assert opened == ["http://127.0.0.1:8765/?token=tok"] + CliRunner().invoke(cli, ["open", "--port", str(listening)]) + assert opened == [f"http://127.0.0.1:{listening}/?token=tok"] def test_a_browser_that_will_not_launch_does_not_fail_the_command( - home: Path, monkeypatch: pytest.MonkeyPatch + home: Path, monkeypatch: pytest.MonkeyPatch, listening: int ) -> None: """`serve` treats the launch as best-effort for the same reason: the URL is already printed, and typing it in is a complete fallback.""" - runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + runtime.record_serving(host="127.0.0.1", port=listening, token="tok", interactive=False) import keel.commands.open_console as open_mod def _boom(_url: str) -> bool: raise RuntimeError("no browser here") monkeypatch.setattr(open_mod.webbrowser, "open", _boom) - result = CliRunner().invoke(cli, ["open", "--port", "8765"]) + result = CliRunner().invoke(cli, ["open", "--port", str(listening)]) assert result.exit_code == 0, result.output - assert "http://127.0.0.1:8765/?token=tok" in result.output + assert f"http://127.0.0.1:{listening}/?token=tok" in result.output + + +# -- review findings (#759) ---------------------------------------------------------------------- + + +def _a_closed_port() -> int: + """A port nothing is listening on: bound to get a free number, then released. + + NOT a hard-coded 8765. The first cut used it and failed on the author's own machine, where a + real `keel serve` was running -- a test that passes only where the feature is unused is worse + than no test. + """ + import socket + + probe = socket.socket() + probe.bind(("127.0.0.1", 0)) + port = int(probe.getsockname()[1]) + probe.close() + return port + + +def test_a_recycled_pid_does_not_make_a_dead_server_look_alive(home: Path) -> None: + """A pid check alone is not liveness. + + keel dies, the OS hands its pid to something else, and the record reads as live -- so `open` + prints a token the server no longer honours and the browser gets a 403. That is exactly the + "keel is broken rather than stopped" confusion the check exists to prevent, and the original + spec asked for a process active ON THE DESIGNATED PORT. + + This record's pid is THIS process, which is certainly alive and certainly not serving. + """ + port = _a_closed_port() + runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False) + assert runtime.live_record(port) is None, "a live pid was accepted with nothing on the port" + + +def test_a_server_that_is_actually_listening_is_offered(home: Path) -> None: + """The other half. A port check that refused everything would pass the test above and break + the feature outright, so the same record is accepted once something is really bound.""" + import socket + + listener = socket.socket() + listener.bind(("127.0.0.1", 0)) + # Backlog room for more than one probe: `live_record` connects once per call and this test + # calls it twice, and nothing here ever `accept()`s, so a backlog of 1 refused the second. + listener.listen(8) + port = int(listener.getsockname()[1]) + try: + runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False) + assert runtime.live_record(port) is not None + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert result.exit_code == 0, result.output + assert "token=tok" in result.output + finally: + listener.close() diff --git a/tests/web/test_runtime_record.py b/tests/web/test_runtime_record.py index 7482c00..fd035fa 100644 --- a/tests/web/test_runtime_record.py +++ b/tests/web/test_runtime_record.py @@ -126,3 +126,44 @@ def test_an_ipv6_host_is_bracketed_like_serve_prints_it(home: Path) -> None: record = runtime.read_record(8765) assert record is not None assert runtime.url_for(record) == "http://[::1]:8765/?token=tok" + + +# -- review findings (#759) ---------------------------------------------------------------------- + + +def test_the_file_is_created_at_0600_rather_than_corrected_afterwards( + home: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`write_text` then `chmod` leaves the token world-readable for the window between them. + + Measured under `umask 022`, a `write_text` file is born `0644`. The `0700` directory means no + other account can traverse in, so the window was not exploitable -- but that is an incidental + mitigation, and creating a secret file at the mode it needs is one argument to `os.open`. + CodeQL points at this same line. + + THE TEST WORKS BY REMOVING THE SAFETY NET: with `chmod` neutered, a file that is merely + corrected afterwards shows its umask mode, and one that is created correctly still shows + `0600`. + """ + monkeypatch.setattr(os, "chmod", lambda *args, **kwargs: None) + monkeypatch.setattr(os, "umask", lambda mask: 0o022) + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + mode = stat.S_IMODE(runtime.record_path(8765).stat().st_mode) + assert mode == 0o600, f"created at {oct(mode)}; the mode must not depend on a later chmod" + + +def test_recording_never_brings_a_deployment_root_into_existence( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`state_root`: "it never creates a deployment folder, because a deployment folder that does + not exist is not one this function chose." + + `mkdir(parents=True)` did exactly that. It is the same hazard `server.ensure_schema` refuses + one directory over -- "a read-only view would bring a deployment into existence merely by + being started" -- and a first-run `keel serve` on a machine with no deployment is a supported + state, not an error. + """ + missing = tmp_path / "no-such-deployment" + monkeypatch.setenv("KEEL_HOME", str(missing)) + assert runtime.record_serving(host="127.0.0.1", port=8765, token="t", interactive=False) is None + assert not missing.exists(), "serving created a deployment root as a side effect"