From bb7a68be9f1579c04dfe2a503c5df50c2d5e8e8e Mon Sep 17 00:00:00 2001 From: garethx Date: Tue, 11 Aug 2026 15:41:54 +0100 Subject: [PATCH] Make a CLI/API-key project mismatch diagnosable instead of silent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #4. In cli mode "which project" has two independent answers: HOOKDECK_API_KEY, which every `hermes hookdeck` command and the dashboard act on, and the CLI's own config, which is what `hookdeck listen` forwards from. Nothing reconciled them, and when they differed the gateway logged that it was listening while receiving nothing — the tunnel restart-looping at warning level out of sight. Three changes, matching the issue's first two suggestions: * `doctor` now compares them and names both project ids. There is no endpoint that reports a key's project, but every resource carries `team_id`, so the connections doctor already lists answer it without an extra request. A key reaching no connections is reported as unverified rather than as a mismatch, since a project can legitimately be empty. A CLI with no session at all is its own failure — `hookdeck listen` cannot start. * The tunnel supervisor escalates a standing failure. Three consecutive runs too short to be healthy now log once at error level with the CLI's own last lines and, for the failures that retrying cannot fix, the likely cause. The backoff is for network blips; `no connection found matching filter` is not one, and at warning level it read as routine churn. A healthy run re-arms it, so a second outage is not silent. * `cli_config` passes `--hookdeck-config` to `hookdeck listen`, making the CLI's project a plugin setting rather than ambient state shared with everything else on the machine. The issue's third suggestion — having the gateway populate that config itself — does not work as described, and the docs say so: the CLI refuses a project API key as a session key ("your API key is invalid or expired"), so only `hookdeck ci` can mint one, and that is the command that rewrites the shared config in the first place. Also fixes a test that passed only on a machine with a CLI session: the doctor fixture now owns the config path, where before it read the developer's real one and would have failed in CI. Co-Authored-By: Claude Opus 5 --- docs/limitations.md | 11 +++ examples/config.yaml | 15 +++++ hookdeck/adapter.py | 1 + hookdeck/cli.py | 91 ++++++++++++++++++++++++- hookdeck/cliconfig.py | 86 ++++++++++++++++++++++++ hookdeck/settings.py | 8 +++ hookdeck/tunnel.py | 81 ++++++++++++++++++++++ tests/test_cli.py | 125 +++++++++++++++++++++++++++++++++- tests/test_cliconfig.py | 78 +++++++++++++++++++++ tests/test_settings.py | 18 +++++ tests/test_tunnel.py | 145 ++++++++++++++++++++++++++++++++++++++++ 11 files changed, 655 insertions(+), 4 deletions(-) create mode 100644 hookdeck/cliconfig.py create mode 100644 tests/test_cliconfig.py diff --git a/docs/limitations.md b/docs/limitations.md index fc70d7d..1a0c132 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -9,6 +9,17 @@ including those that only surface once you provision connections yourself. or alerting. - CLI mode runs one `hookdeck listen` process per route. That is fine for a handful; a gateway with dozens of routes wants push mode. +- In CLI mode the CLI's project and the API key's project are separate pieces + of state, and only the operator reconciles them. `hermes hookdeck setup`, + `status`, `retry`, the dashboard tab and the agent tools all act on the + project `HOOKDECK_API_KEY` belongs to; `hookdeck listen` forwards from + whichever project `~/.config/hookdeck/config.toml` records. When they + differ the gateway starts, reports healthy, and receives nothing — every + event is ignored as `CLI_DISCONNECTED` while the tunnel restart-loops. Run + `hermes hookdeck doctor`, which compares the two and names both project ids. + `cli_config` points the CLI at a config of the gateway's own, but populating + one still needs `hookdeck ci`: the CLI does not accept a project API key as + a session key. - `setup` only pushes an `events` filter down into Hookdeck when it knows where the event name lives: a header for GitHub, GitLab and Shopify, or a body path you set with `event_path`. Otherwise the filter stays adapter-side, because a diff --git a/examples/config.yaml b/examples/config.yaml index 5a44c2a..acfe2b0 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -53,6 +53,21 @@ gateway: # gateway should do to a shared tool. Run `hookdeck login` instead. # cli_login: false + # Which CLI config `hookdeck listen` reads, via --hookdeck-config. + # Unset, it uses ~/.config/hookdeck/config.toml — shared with every + # other Hookdeck CLI use on the machine, and its active project is + # ambient state this gateway does not control. That project is what + # `hookdeck listen` forwards from, and it is entirely independent of + # HOOKDECK_API_KEY: when the two differ, `hermes hookdeck setup` + # creates the connection in one project while the tunnel looks for it + # in the other, and the gateway reports healthy while receiving + # nothing. `hermes hookdeck doctor` compares them and says so. + # + # Note that populating a gateway-owned config still requires + # `hookdeck ci`, because the CLI will not accept a project API key as + # a session key. + # cli_config: ~/.hermes/hookdeck-cli.toml + routes: # A GitHub PR reviewer. `source` is the Hookdeck source name. github-prs: diff --git a/hookdeck/adapter.py b/hookdeck/adapter.py index e35ddb8..2cf1f4d 100644 --- a/hookdeck/adapter.py +++ b/hookdeck/adapter.py @@ -294,6 +294,7 @@ async def _start_tunnels(self) -> bool: connection_name=route_name, binary=self.settings.cli_binary, login=self.settings.cli_login, + config_path=self.settings.cli_config, ) await tunnel.start() self._tunnels.append(tunnel) diff --git a/hookdeck/cli.py b/hookdeck/cli.py index ad6bfb1..191a600 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -18,6 +18,7 @@ from typing import Any, Optional from .api import HookdeckAPI, HookdeckAPIError, run_sync +from .cliconfig import DEFAULT_CLI_CONFIG, cli_project_id from .constants import DEFAULT_PATH, DEFAULT_PORT from .provision import ( build_connection_payload, @@ -442,6 +443,17 @@ def _check_routes(routes: dict) -> Check: ) +def _cli_config_path(extra: dict) -> Path: + """Where the CLI's session lives, honouring an override. + + ``hookdeck listen`` takes ``--hookdeck-config``, so an operator who points + the gateway at a config of its own must have doctor read the same one — + otherwise the check reports on a file nothing uses. + """ + configured = extra.get("cli_config") + return Path(configured).expanduser() if configured else DEFAULT_CLI_CONFIG + + def _check_cli(extra: dict) -> list[Check]: """The CLI is only reachable in cli mode, and only the resolved one matters.""" configured = extra.get("cli_binary") or "hookdeck" @@ -505,7 +517,79 @@ def _report_stranded_runs() -> None: ledger.close() -async def _check_live_connections(routes: dict) -> list[Check]: +async def _api_project_id( + api: HookdeckAPI, already_found: list[dict] +) -> Optional[str]: + """Which project the API key belongs to. + + There is no endpoint that answers this directly, but every resource the key + can reach carries ``team_id`` — so the connections the caller already + listed answer it for free, and only an empty result needs a request of its + own. ``None`` means the key reaches no connections at all, which is not + itself a fault: a project can legitimately be empty. + """ + for connection in already_found: + if connection.get("team_id"): + return str(connection["team_id"]) + + result = await api.list_connections(limit=1) + models = (result or {}).get("models") or (result or {}).get("data") or [] + for connection in models: + if connection.get("team_id"): + return str(connection["team_id"]) + return None + + +async def _check_cli_project( + api: HookdeckAPI, extra: dict, already_found: list[dict] +) -> Check: + """Whether the CLI forwards from the project the API key acts on. + + These are two unrelated pieces of state — an env var and + ``~/.config/hookdeck/config.toml`` — and nothing else reconciles them. When + they differ, `setup` creates the connection in one project while + ``hookdeck listen`` looks for it in the other: the gateway logs that it is + listening, the tunnel restart-loops out of sight, and every event is + ignored as ``CLI_DISCONNECTED``. From the outside that is indistinguishable + from "no events are arriving", which is the whole class of problem this + command exists to make diagnosable. + """ + config_path = _cli_config_path(extra) + cli_project = cli_project_id(config_path) + api_project = await _api_project_id(api, already_found) + + if cli_project is None: + return Check( + False, + f"No Hookdeck CLI session found at {config_path} — `hookdeck " + "listen` cannot start, so no events will reach the gateway. Run " + "`hookdeck login`.", + ) + if api_project is None: + return Check( + True, + f"Hookdeck CLI is logged into project {cli_project}", + note="The API key's project could not be determined, because it " + "reaches no connections yet — so the two are unverified. Re-run " + "this after `hermes hookdeck setup`.", + ) + if cli_project != api_project: + return Check( + False, + f"Project mismatch: the CLI forwards from {cli_project} but the " + f"API key acts on {api_project}. `hermes hookdeck setup` creates " + "connections in the API key's project while `hookdeck listen` " + "looks for them in the CLI's, so the gateway will report healthy " + "and receive nothing.", + note="Point the CLI at the same project with `hookdeck login`, or " + "set the API key to one belonging to " + cli_project + ".", + ) + return Check(True, f"CLI and API key agree on project {api_project}") + + +async def _check_live_connections( + routes: dict, *, mode: str, extra: dict +) -> list[Check]: """Reachability, plus whether each retry rule covers what the adapter emits. A rule narrower than the emitted statuses is silent data loss — a deferred @@ -523,6 +607,9 @@ async def _check_live_connections(routes: dict) -> list[Check]: result = await api.list_connections(name=route_name, limit=10) found += (result or {}).get("models") or (result or {}).get("data") or [] + if mode == "cli": + checks.append(await _check_cli_project(api, extra, found)) + for connection in found: if connection.get("name") not in routes: continue @@ -575,7 +662,7 @@ def _cmd_doctor(_args: argparse.Namespace) -> int: if os.getenv("HOOKDECK_API_KEY"): print() try: - live = run_sync(_check_live_connections(routes)) + live = run_sync(_check_live_connections(routes, mode=mode, extra=extra)) except HookdeckAPIError as exc: live = [Check(False, f"Hookdeck API check failed: {exc}")] for check in live: diff --git a/hookdeck/cliconfig.py b/hookdeck/cliconfig.py new file mode 100644 index 0000000..de9e63c --- /dev/null +++ b/hookdeck/cliconfig.py @@ -0,0 +1,86 @@ +"""Reading the Hookdeck CLI's own configuration. + +In ``cli`` mode there are two independent notions of "which project": the API +key, which every ``hermes hookdeck`` command and the dashboard act on, and the +CLI's stored session, which is what ``hookdeck listen`` actually forwards from. +Nothing reconciles them, and when they differ the gateway reports healthy while +no event ever arrives — the tunnel restart-loops out of sight, and the source +gets auto-created in the wrong project on the way. + +This module is the read-only half of catching that: it reports which project +the CLI would use. Nothing here writes, because ``hookdeck ci`` — the only way +to change it — rewrites the operator's shared config and switches its active +project, which is not something starting a gateway should do. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +#: Where the CLI keeps its session unless told otherwise, matching the CLI's +#: own ``--hookdeck-config`` default. +DEFAULT_CLI_CONFIG = Path.home() / ".config" / "hookdeck" / "config.toml" + + +def _parse_toml(text: str) -> dict: + """Parse the CLI config, by whatever means this interpreter has. + + ``tomllib`` is stdlib from 3.11 and this package supports 3.10, so there is + a fallback. The file is a handful of ``key = "value"`` lines under one + table header, so the fallback covers it — and a diagnostic that degrades is + better than one that raises on the older interpreter. + """ + try: + import tomllib + except ImportError: # pragma: no cover - only on 3.10 + return _parse_flat_toml(text) + try: + return tomllib.loads(text) + except Exception: # noqa: BLE001 - a malformed config is "unknown", not fatal + return _parse_flat_toml(text) + + +_ASSIGNMENT = re.compile(r"""^\s*([A-Za-z0-9_-]+)\s*=\s*(.+?)\s*$""") +_TABLE = re.compile(r"""^\s*\[([^\]]+)\]\s*$""") + + +def _parse_flat_toml(text: str) -> dict: + """Enough TOML for this one file: top-level keys and single-level tables.""" + result: dict = {} + table = result + for line in text.splitlines(): + if not line.strip() or line.lstrip().startswith("#"): + continue + header = _TABLE.match(line) + if header: + table = result.setdefault(header.group(1).strip(), {}) + continue + assignment = _ASSIGNMENT.match(line) + if assignment: + table[assignment.group(1)] = assignment.group(2).strip().strip('"\'') + return result + + +def cli_project_id(path: Path | None = None) -> str | None: + """The project ``hookdeck listen`` would forward from. + + ``None`` when it cannot be determined — no config, unreadable, or no + project recorded. That is reported as "unknown" rather than as a mismatch, + because guessing here would be worse than saying so. + """ + config_path = DEFAULT_CLI_CONFIG if path is None else path + try: + text = config_path.read_text() + except OSError: + return None + + parsed = _parse_toml(text) + # The CLI supports named profiles and records which one is active at the + # top level; each profile is a table of its own. + profile = parsed.get("profile") or "default" + section = parsed.get(profile) + if not isinstance(section, dict): + return None + project = section.get("project_id") + return str(project) if project else None diff --git a/hookdeck/settings.py b/hookdeck/settings.py index d8c590f..c1a8176 100644 --- a/hookdeck/settings.py +++ b/hookdeck/settings.py @@ -122,6 +122,11 @@ class AdapterSettings: ledger_ttl_seconds: float = DEFAULT_LEDGER_TTL_SECONDS max_body_bytes: int = DEFAULT_MAX_BODY_BYTES cli_binary: str = "hookdeck" + #: Path passed to `hookdeck listen --hookdeck-config`. Empty means the + #: CLI's shared default, whose active project is ambient state the gateway + #: does not control — `hermes hookdeck doctor` reports when that disagrees + #: with the API key's project. + cli_config: str = "" cli_login: bool = False # ------------------------------------------------------------------ @@ -186,6 +191,9 @@ def text(key: str, env: str = "", default: str = "") -> str: # An npm global shadowing a Homebrew install is the common case, # and PATH silently picks the older one. cli_binary=text("cli_binary", default="hookdeck"), + cli_config=str( + Path(extra["cli_config"]).expanduser() if extra.get("cli_config") else "" + ), # Off by default: `hookdeck ci` rewrites the shared CLI config and # repoints its active project — not something starting a gateway # should do to a tool the operator uses for other work. diff --git a/hookdeck/tunnel.py b/hookdeck/tunnel.py index 119b508..5cc5be4 100644 --- a/hookdeck/tunnel.py +++ b/hookdeck/tunnel.py @@ -29,6 +29,47 @@ # Generous enough that CLI output never kills a working tunnel. _STDOUT_LINE_LIMIT = 1024 * 1024 +# How many consecutive too-short runs before the restart loop is treated as a +# standing failure rather than a blip. Two would fire on an ordinary flap; this +# is small enough to be prompt and large enough not to cry wolf. +_FAST_FAILURES_BEFORE_ESCALATING = 3 + +# How much of the CLI's output to keep for diagnosis. The useful line is the +# last one before it exits. +_OUTPUT_MEMORY = 12 + +#: Substrings the CLI prints for failures that retrying cannot fix, paired with +#: what to do about them. The backoff exists for network blips; these need a +#: person, and without this they scroll past at warning level for ever. +_DETERMINISTIC_FAILURES: tuple[tuple[str, str], ...] = ( + ( + "no connection found matching filter", + "the CLI is logged into a different Hookdeck project than the API key " + "used by `hermes hookdeck setup`, or setup has not been run for this " + "route. Run `hermes hookdeck doctor` — it compares the two.", + ), + ( + "automatically creating source", + "the source does not exist in the project the CLI is logged into, so " + "the CLI has just created a stray one there. That is usually a " + "different project than the API key acts on. Run `hermes hookdeck " + "doctor`.", + ), + ( + "authentication failed", + "the CLI's stored session is invalid or expired. Run `hookdeck login`.", + ), +) + + +def diagnose(lines: list[str]) -> str: + """A likely cause for the CLI's exit, or "" if none is recognised.""" + haystack = "\n".join(lines).lower() + for marker, explanation in _DETERMINISTIC_FAILURES: + if marker in haystack: + return explanation + return "" + class HookdeckCLIMissing(RuntimeError): """The ``hookdeck`` binary is not on PATH.""" @@ -54,6 +95,7 @@ def __init__( api_key: str = "", binary: str = "hookdeck", login: bool = False, + config_path: str = "", ): if not source: raise ValueError( @@ -68,9 +110,16 @@ def __init__( self._api_key = api_key or os.getenv("HOOKDECK_API_KEY", "") self._login_enabled = login self._binary = binary + # Which CLI config `hookdeck listen` reads. Empty means the CLI's own + # default, which is the operator's shared session — see #4 for why + # that being ambient is a problem worth being able to opt out of. + self._config_path = config_path self._process: Optional[asyncio.subprocess.Process] = None self._supervisor: Optional[asyncio.Task] = None self._stopping = False + #: Tail of the last run's output, kept so a failing restart loop can + #: say *why* rather than only that it is looping. + self._recent_output: list[str] = [] # ------------------------------------------------------------------ # Command construction @@ -99,6 +148,8 @@ def listen_args(self) -> list[str]: args.append(self._connection_name) if self._path: args += ["--path", self._path] + if self._config_path: + args += ["--hookdeck-config", self._config_path] # The default `interactive` output renders a full-screen UI and exits # immediately when stdout is not a TTY — which it never is here, since # the supervisor pipes it into the gateway log. @@ -176,6 +227,8 @@ async def _login(self, binary: str) -> None: async def _supervise(self, binary: str) -> None: backoff = _BACKOFF_INITIAL + fast_failures = 0 + escalated = False while not self._stopping: started = asyncio.get_running_loop().time() try: @@ -191,6 +244,28 @@ async def _supervise(self, binary: str) -> None: ran_for = asyncio.get_running_loop().time() - started if ran_for >= _HEALTHY_RUN_SECONDS: backoff = _BACKOFF_INITIAL + fast_failures = 0 + escalated = False + else: + fast_failures += 1 + + # A tunnel that never stays up is not retrying its way out of + # anything, and at warning level it reads as routine churn. Say so + # once, loudly, with the cause if the CLI named one — then fall + # back to the quiet line so the log does not fill up. + if fast_failures >= _FAST_FAILURES_BEFORE_ESCALATING and not escalated: + escalated = True + cause = diagnose(self._recent_output) + logger.error( + "[hookdeck] CLI tunnel has failed to stay up %d times in a " + "row (last run %.0fs). No events are reaching the gateway. " + "%sLast output: %s", + fast_failures, + ran_for, + f"Likely cause: {cause} " if cause else "", + " / ".join(self._recent_output[-3:]) or "(none)", + ) + logger.warning( "[hookdeck] CLI tunnel exited after %.0fs — restarting in %.0fs", ran_for, @@ -213,11 +288,17 @@ async def _run_once(self, binary: str) -> None: limit=_STDOUT_LINE_LIMIT, env={**os.environ, **({"HOOKDECK_API_KEY": self._api_key} if self._api_key else {})}, ) + # Reset per run: the diagnosis is about why *this* run ended, and + # carrying lines over from the previous one would name a cause that has + # since been fixed. + self._recent_output = [] assert self._process.stdout is not None async for line in self._process.stdout: text = line.decode("utf-8", "replace").rstrip() if text: logger.info("[hookdeck cli] %s", text) + self._recent_output.append(text) + del self._recent_output[:-_OUTPUT_MEMORY] await self._process.wait() async def _terminate(self) -> None: diff --git a/tests/test_cli.py b/tests/test_cli.py index 4e6cb8c..dbe7281 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -376,17 +376,24 @@ def test_a_failing_retry_exits_non_zero(fake_api, capsys): @pytest.fixture() -def doctor_env(monkeypatch, ledger_at): - """A doctor run with nothing live: no API key, no CLI binary. +def doctor_env(monkeypatch, ledger_at, tmp_path): + """A doctor run with nothing live: no API key, no CLI binary, no session. Both env-var spellings are cleared. Only the unprefixed names are read on this branch, but an operator's shell may have either set, and a doctor test that passes because of the caller's environment is worse than none. + + The CLI config path is redirected for the same reason: read from the real + location, these tests pass on a developer's laptop — where a session + exists — and fail in CI, where none does. """ for name in ("API_KEY", "WEBHOOK_SECRET", "MODE", "PROJECT_ID"): monkeypatch.delenv(f"HOOKDECK_{name}", raising=False) monkeypatch.delenv(f"HOOKDECK_EG_{name}", raising=False) + monkeypatch.cli_config = tmp_path / "hookdeck-cli.toml" + monkeypatch.setattr(cli, "_cli_config_path", lambda _extra: monkeypatch.cli_config) + # Recorded, not just stubbed: "did the CLI checks run at all?" is the # question in push mode, and answering it by grepping the output for # "Hookdeck CLI" breaks as soon as an unrelated message mentions the CLI. @@ -402,6 +409,16 @@ def which(binary): return monkeypatch +def _write_cli_config(doctor_env, *, project_id, profile="default"): + """Give the fixture's CLI config a logged-in session.""" + doctor_env.cli_config.write_text( + f'profile = "{profile}"\n\n' + f"[{profile}]\n" + 'api_key = "cli_session_key"\n' + f'project_id = "{project_id}"\n' + ) + + def _configure(monkeypatch, *, routes=None, **extra): """Set both config readers doctor uses, so they cannot disagree.""" full = {**extra, "routes": routes or {}} @@ -449,6 +466,7 @@ def test_doctor_passes_when_everything_is_in_place( "models": [ { "name": "github", + "team_id": "tm_same", "rules": [ { "type": "retry", @@ -458,6 +476,7 @@ def test_doctor_passes_when_everything_is_in_place( } ] } + _write_cli_config(doctor_env, project_id="tm_same") _configure(doctor_env, secret="whsec_x", routes={"github": {"source": "github"}}) assert cli.hookdeck_command(_ns("doctor")) == 0 @@ -553,3 +572,105 @@ def test_doctor_reports_an_unreachable_api_as_a_failed_check( _configure(doctor_env, routes={"github": {}}) assert cli.hookdeck_command(_ns("doctor")) == 1 assert "Hookdeck API check failed" in capsys.readouterr().out + + +# ── doctor: the two notions of "which project" (#4) ───────────────── + + +def _live_doctor(doctor_env, fake_api, monkeypatch, *, connections): + """A doctor run in cli mode with an API key, so the live checks run.""" + monkeypatch.setenv("HOOKDECK_API_KEY", "key") + monkeypatch.setenv("HOOKDECK_WEBHOOK_SECRET", "whsec_x") + doctor_env.setattr(cli.shutil, "which", lambda _b: "/usr/local/bin/hookdeck") + doctor_env.setattr(cli, "_cli_version", lambda _b: "2.4.0") + doctor_env.setattr(cli, "_other_hookdeck_binaries", lambda _r: []) + fake_api.responses["list_connections"] = {"models": connections} + _configure(doctor_env, secret="whsec_x", routes={"github": {"source": "github"}}) + return cli.hookdeck_command(_ns("doctor")) + + +def test_doctor_catches_the_cli_and_the_api_key_pointing_at_different_projects( + doctor_env, fake_api, monkeypatch, capsys +): + # The failure this exists for: `setup` creates the connection in the API + # key's project while `hookdeck listen` looks for it in the CLI's. The + # gateway logs that it is listening and receives nothing, which looks + # exactly like "no events are arriving". + _write_cli_config(doctor_env, project_id="tm_cli") + code = _live_doctor( + doctor_env, fake_api, monkeypatch, + connections=[{"name": "github", "team_id": "tm_apikey"}], + ) + out = capsys.readouterr().out + assert code == 1 + assert "Project mismatch" in out + # Both ids named, because the fix is to change one of them and the + # operator has to know which is which. + assert "tm_cli" in out and "tm_apikey" in out + assert "hookdeck login" in out + + +def test_doctor_is_quiet_when_both_agree(doctor_env, fake_api, monkeypatch, capsys): + _write_cli_config(doctor_env, project_id="tm_same") + code = _live_doctor( + doctor_env, fake_api, monkeypatch, + connections=[{"name": "github", "team_id": "tm_same"}], + ) + out = capsys.readouterr().out + assert code == 0 + assert "agree on project tm_same" in out + assert "mismatch" not in out.lower() + + +def test_doctor_reports_a_cli_that_was_never_logged_in( + doctor_env, fake_api, monkeypatch, capsys +): + # No config file at all. `hookdeck listen` cannot start, so this is a + # failure in its own right rather than an unknown. + code = _live_doctor( + doctor_env, fake_api, monkeypatch, + connections=[{"name": "github", "team_id": "tm_apikey"}], + ) + out = capsys.readouterr().out + assert code == 1 + assert "No Hookdeck CLI session found" in out + assert "hookdeck login" in out + + +def test_an_empty_project_is_unknown_rather_than_a_mismatch( + doctor_env, fake_api, monkeypatch, capsys +): + # A key that reaches no connections cannot be placed. Reporting that as a + # mismatch would send an operator to fix something that is not broken. + _write_cli_config(doctor_env, project_id="tm_cli") + code = _live_doctor(doctor_env, fake_api, monkeypatch, connections=[]) + out = capsys.readouterr().out + assert code == 0 + assert "logged into project tm_cli" in out + assert "could not be determined" in out + + +def test_the_project_check_does_not_run_in_push_mode( + doctor_env, fake_api, monkeypatch, capsys +): + # Push mode does not run `hookdeck listen`, so the CLI's project is + # irrelevant and reporting on it would be noise an operator must dismiss. + monkeypatch.setenv("HOOKDECK_API_KEY", "key") + _write_cli_config(doctor_env, project_id="tm_cli") + fake_api.responses["list_connections"] = { + "models": [{"name": "github", "team_id": "tm_apikey"}] + } + _configure(doctor_env, mode="push", public_url="https://example.com", + secret="whsec_x", routes={"github": {}}) + cli.hookdeck_command(_ns("doctor")) + out = capsys.readouterr().out + assert "Project mismatch" not in out + assert "CLI session" not in out + + +def test_the_project_is_read_from_the_configured_cli_config(tmp_path): + # `hookdeck listen --hookdeck-config` lets the config move; doctor reading + # the default path anyway would report on a file nothing uses. + custom = tmp_path / "gateway-cli.toml" + assert cli._cli_config_path({"cli_config": str(custom)}) == custom + assert cli._cli_config_path({}) == cli.DEFAULT_CLI_CONFIG diff --git a/tests/test_cliconfig.py b/tests/test_cliconfig.py new file mode 100644 index 0000000..ddfa12c --- /dev/null +++ b/tests/test_cliconfig.py @@ -0,0 +1,78 @@ +"""Reading the Hookdeck CLI's session config. + +The project recorded here is what `hookdeck listen` forwards from, and it is +entirely independent of the API key. Misreading it produces a false "projects +agree", which is worse than no check at all. +""" + +from __future__ import annotations + +from hookdeck.cliconfig import _parse_flat_toml, cli_project_id + +REAL_SHAPE = '''profile = "default" + +[default] +api_key = "cli_abc" +guest_url = "" +project_id = "tm_WYrcWDMnjZpG" +project_mode = "console" +project_type = "team" +''' + + +def test_the_project_is_read_from_the_active_profile(tmp_path): + path = tmp_path / "config.toml" + path.write_text(REAL_SHAPE) + assert cli_project_id(path) == "tm_WYrcWDMnjZpG" + + +def test_a_named_profile_wins_over_the_default_table(tmp_path): + # `hookdeck --profile` writes a second table and repoints `profile`. Always + # reading [default] would report a project the CLI is not using. + path = tmp_path / "config.toml" + path.write_text( + 'profile = "work"\n\n' + '[default]\nproject_id = "tm_personal"\n\n' + '[work]\nproject_id = "tm_work"\n' + ) + assert cli_project_id(path) == "tm_work" + + +def test_a_missing_file_is_unknown_rather_than_an_error(tmp_path): + assert cli_project_id(tmp_path / "absent.toml") is None + + +def test_a_config_with_no_project_is_unknown(tmp_path): + path = tmp_path / "config.toml" + path.write_text('profile = "default"\n\n[default]\napi_key = "cli_abc"\n') + assert cli_project_id(path) is None + + +def test_a_profile_pointing_at_no_table_is_unknown(tmp_path): + path = tmp_path / "config.toml" + path.write_text('profile = "ghost"\n\n[default]\nproject_id = "tm_x"\n') + assert cli_project_id(path) is None + + +def test_garbage_is_unknown_rather_than_a_crash(tmp_path): + # doctor runs when things are already broken; a corrupt config must not be + # the thing that stops it reporting everything else. + path = tmp_path / "config.toml" + path.write_text("\x00\x01 not toml at all [[[") + assert cli_project_id(path) is None + + +def test_the_fallback_parser_handles_this_files_shape(): + # Exercised directly, because on 3.11+ tomllib means the fallback is never + # reached by the tests above — and 3.10 is a supported interpreter. + parsed = _parse_flat_toml(REAL_SHAPE) + assert parsed["profile"] == "default" + assert parsed["default"]["project_id"] == "tm_WYrcWDMnjZpG" + + +def test_the_fallback_parser_ignores_comments_and_blank_lines(): + parsed = _parse_flat_toml( + '# a comment\n\nprofile = "p"\n\n[p]\n # indented comment\n' + ' project_id = "tm_1"\n' + ) + assert parsed["p"]["project_id"] == "tm_1" diff --git a/tests/test_settings.py b/tests/test_settings.py index 969fb3c..f211059 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -112,3 +112,21 @@ def test_the_default_is_used_when_nothing_is_configured(monkeypatch): monkeypatch.setattr("hookdeck.settings.load_hermes_config", dict) assert configured_state_path() == default_state_path() + + +def test_a_gateway_owned_cli_config_is_resolved_to_an_absolute_path(): + from hookdeck.settings import AdapterSettings + + settings = AdapterSettings.from_extra( + {"secret": "s", "source": "src", "cli_config": "~/hermes/hookdeck.toml"} + ) + assert settings.cli_config.endswith("/hermes/hookdeck.toml") + assert "~" not in settings.cli_config + + +def test_the_cli_config_defaults_to_the_clis_own_default(): + from hookdeck.settings import AdapterSettings + + # Empty rather than a path: the flag is then omitted entirely, so the CLI + # applies its own default rather than being handed our guess at it. + assert AdapterSettings.from_extra({"secret": "s"}).cli_config == "" diff --git a/tests/test_tunnel.py b/tests/test_tunnel.py index 8d32f32..4c2f6d5 100644 --- a/tests/test_tunnel.py +++ b/tests/test_tunnel.py @@ -329,3 +329,148 @@ async def communicate_ok(self): assert "rewrites" in caplog.text assert spawned.commands[0][0][1:] == ("ci", "--api-key", "k") + + +# ---------------------------------------------------------------------- +# Escalating a failure that retrying cannot fix (#4) +# ---------------------------------------------------------------------- + +#: The output observed in #4, verbatim. The project the CLI is logged into has +#: no such source, so it invents one and then finds no connection for it. +PROJECT_MISMATCH_OUTPUT = [ + b'Source "hermes-livetest" not found.\n', + b'Non-interactive mode detected. Automatically creating source "hermes-livetest".\n', + b'no connection found matching filter "livetest" for source "hermes-livetest"\n', +] + + +def test_a_project_mismatch_is_recognised_from_the_cli_output(): + cause = tunnel_mod.diagnose( + [line.decode().strip() for line in PROJECT_MISMATCH_OUTPUT] + ) + assert "different Hookdeck project" in cause + assert "hermes hookdeck doctor" in cause + + +def test_an_expired_session_is_recognised(): + assert "hookdeck login" in tunnel_mod.diagnose( + ["Authentication failed: your API key is invalid or expired."] + ) + + +def test_output_with_no_known_cause_diagnoses_nothing(): + # Better to say only what is known than to guess a cause and send an + # operator after the wrong thing. + assert tunnel_mod.diagnose(["some unrecognised failure"]) == "" + assert tunnel_mod.diagnose([]) == "" + + +async def test_a_tunnel_stuck_in_a_restart_loop_escalates_and_names_the_cause( + spawned, no_waiting, caplog +): + # #4: the gateway logs that it is listening, the tunnel fails identically + # every couple of seconds at warning level, and nothing ever says the + # gateway is receiving no events. + for _ in range(10): + spawned.queue.append(FakeProcess(lines=list(PROJECT_MISMATCH_OUTPUT))) + + tunnel = HookdeckTunnel(port=1, path="/x", source="hermes-livetest") + with caplog.at_level(logging.INFO): + await _supervise_rounds(tunnel, 5, no_waiting) + + errors = [r.getMessage() for r in caplog.records if r.levelno >= logging.ERROR] + assert len(errors) == 1, "escalate once per streak, not on every restart" + assert "No events are reaching the gateway" in errors[0] + assert "different Hookdeck project" in errors[0] + # The operator needs the CLI's own words too, not just our interpretation. + assert "no connection found matching filter" in errors[0] + + +async def test_escalation_waits_for_a_streak_rather_than_one_bad_start( + spawned, no_waiting, caplog +): + # A single fast exit happens on an ordinary flap. Crying wolf there would + # make the error level meaningless for the case that matters. + for _ in range(10): + spawned.queue.append(FakeProcess(lines=list(PROJECT_MISMATCH_OUTPUT))) + + tunnel = HookdeckTunnel(port=1, path="/x", source="s") + await _supervise_rounds(tunnel, tunnel_mod._FAST_FAILURES_BEFORE_ESCALATING - 1, + no_waiting) + + assert [r for r in caplog.records if r.levelno >= logging.ERROR] == [] + + +async def test_a_healthy_run_rearms_the_escalation( + spawned, no_waiting, caplog, monkeypatch +): + # A tunnel that recovers and later breaks again deserves to be shouted + # about again — otherwise the second outage is silent. + clock = {"now": 0.0} + + class FakeLoop: + def time(self): + return clock["now"] + + monkeypatch.setattr(tunnel_mod.asyncio, "get_running_loop", lambda: FakeLoop()) + for _ in range(20): + spawned.queue.append(FakeProcess(lines=list(PROJECT_MISMATCH_OUTPUT))) + + tunnel = HookdeckTunnel(port=1, path="/x", source="s") + runs = {"n": 0} + original = tunnel._run_once + + async def timed_run(binary): + runs["n"] += 1 + # Three fast failures, one healthy session, then three more failures. + healthy = runs["n"] == 4 + clock["now"] += tunnel_mod._HEALTHY_RUN_SECONDS * 2 if healthy else 1.0 + await original(binary) + + tunnel._run_once = timed_run + await _supervise_rounds(tunnel, 7, no_waiting) + + errors = [r for r in caplog.records if r.levelno >= logging.ERROR] + assert len(errors) == 2, "once before the recovery, once after it breaks again" + + +async def test_the_diagnosis_does_not_outlive_the_run_that_produced_it(spawned): + # Carrying output across runs would name a cause that has since been + # fixed — the worst kind of wrong, because it looks specific. + tunnel = HookdeckTunnel(port=1, path="/x", source="s") + spawned.queue.append(FakeProcess(lines=list(PROJECT_MISMATCH_OUTPUT))) + await tunnel._run_once("/usr/bin/hookdeck") + assert tunnel_mod.diagnose(tunnel._recent_output) + + spawned.queue.append(FakeProcess(lines=[b"Ready!\n"])) + await tunnel._run_once("/usr/bin/hookdeck") + assert tunnel._recent_output == ["Ready!"] + assert tunnel_mod.diagnose(tunnel._recent_output) == "" + + +async def test_the_output_buffer_is_bounded(spawned): + # A chatty tunnel runs for days; the tail is what diagnoses, not the whole + # session. + tunnel = HookdeckTunnel(port=1, path="/x", source="s") + spawned.queue.append( + FakeProcess(lines=[f"line {i}\n".encode() for i in range(500)]) + ) + await tunnel._run_once("/usr/bin/hookdeck") + assert len(tunnel._recent_output) == tunnel_mod._OUTPUT_MEMORY + assert tunnel._recent_output[-1] == "line 499" + + +async def test_a_gateway_owned_config_is_passed_to_the_cli(): + # `--hookdeck-config` is what makes the CLI's project a plugin setting + # rather than ambient state shared with whatever else the operator runs. + args = HookdeckTunnel( + port=1, path="/x", source="s", config_path="/etc/hermes/hookdeck.toml" + ).listen_args() + assert args[args.index("--hookdeck-config") + 1] == "/etc/hermes/hookdeck.toml" + + +def test_no_config_flag_is_passed_when_none_is_configured(): + # Absent, not empty: `--hookdeck-config ""` would point the CLI at nothing. + assert "--hookdeck-config" not in HookdeckTunnel( + port=1, path="/x", source="s" + ).listen_args()