From 5eeeaa5581695d2c662ab41c7770befd46cb591a Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 11:52:28 +0100 Subject: [PATCH 01/14] Refuse a malformed signature with 401, not 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hmac.compare_digest` rejects two `str` arguments unless both are pure ASCII, and the signature header is a value an unauthenticated sender controls outright. One non-ASCII byte raised `TypeError` out of `verify_signature`, and aiohttp turned that into a 500. Wrong twice over. It bypassed `assert_declared_status`, which every other response goes through precisely so no status escapes without its retryability being declared — so the one status the adapter emitted without deciding anything was the one an attacker could trigger. And 500 is inside the provisioned retry rule's `500-599` range, so Hookdeck retried a forged request roughly ten times with backoff instead of dropping it on the first refusal. Compare as bytes, which has no ASCII restriction. Neither encode can fail: the expected value is base64, and `replace` turns an undecodable candidate into bytes that simply do not match. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit d4ce91eb855e1f924fdc8127f7c1e36691641292) --- hookdeck/verify.py | 17 ++++++++++++++++- tests/test_adapter.py | 18 ++++++++++++++++++ tests/test_verify.py | 14 ++++++++++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/hookdeck/verify.py b/hookdeck/verify.py index b81c7d9..91746f6 100644 --- a/hookdeck/verify.py +++ b/hookdeck/verify.py @@ -29,11 +29,26 @@ def compute_signature(body: bytes, secret: str) -> str: def _matches_any(expected: str, provided: Iterable[str]) -> bool: + """Whether any candidate is *expected*, compared as bytes. + + Bytes rather than ``str`` because ``compare_digest`` refuses two ``str`` + arguments unless both are pure ASCII, and a candidate is a header value an + unauthenticated sender controls outright. One non-ASCII byte in it used to + raise ``TypeError`` straight out of the verification path, which aiohttp + turned into a 500 — wrong twice over: it bypassed the + ``EMITTED_STATUS_RETRYABLE`` guard every other response goes through, and + 500 sits inside the provisioned retry rule's ``500-599`` range, so a + malformed request was retried rather than refused. + + Encoding cannot fail on either side: *expected* is base64, and ``replace`` + maps an undecodable candidate to bytes that simply do not match. + """ + wanted = expected.encode("ascii") # compare_digest on every candidate rather than short-circuiting, so the # work done does not depend on which header happened to match. matched = False for candidate in provided: - if candidate and hmac.compare_digest(expected, candidate): + if candidate and hmac.compare_digest(wanted, candidate.encode("utf-8", "replace")): matched = True return matched diff --git a/tests/test_adapter.py b/tests/test_adapter.py index db804cf..16e02fb 100644 --- a/tests/test_adapter.py +++ b/tests/test_adapter.py @@ -123,6 +123,24 @@ async def test_delivery_signed_with_the_wrong_secret_is_rejected(client_factory) assert response.status == 401 +async def test_a_malformed_signature_is_refused_not_a_server_error(client_factory): + """A junk signature must answer 401, like any other forged one. + + A non-ASCII byte in the header used to raise out of the verification path + and become a 500 — which skips ``assert_declared_status`` entirely and, + because the provisioned retry rule covers ``500-599``, had Hookdeck retry + an unauthenticated request instead of dropping it. + """ + _adapter, client = await client_factory({"default": {}}) + response = await post( + client, + {"hello": "world"}, + sign=False, + extra_headers={"x-hookdeck-signature": "not-base64-é"}, + ) + assert response.status == 401 + + async def test_unmatched_source_returns_404(client_factory): _adapter, client = await client_factory( {"github": {"source": "github"}, "stripe": {"source": "stripe"}} diff --git a/tests/test_verify.py b/tests/test_verify.py index fcd5f02..289ad3f 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -51,6 +51,20 @@ def test_an_empty_secret_never_passes(): assert not verify_signature(signed, BODY, "") +def test_a_non_ascii_signature_is_rejected_not_raised(): + # The sender controls this header outright. compare_digest refuses two + # non-ASCII strs, and letting that TypeError escape turned a forged + # signature into a 500 — which the retry rule then treats as retryable. + assert not verify_signature(headers(**{"x-hookdeck-signature": "abcé"}), BODY, SECRET) + + +def test_a_lone_surrogate_signature_is_rejected_not_raised(): + # Not reachable through aiohttp today, but the guard is one encode call and + # the failure mode it prevents is a 500 on the security-critical path. + signed = headers(**{"x-hookdeck-signature": "\ud800bad"}) + assert not verify_signature(signed, BODY, SECRET) + + def test_honours_a_custom_header_prefix(): signed = headers(**{"x-acme-signature": compute_signature(BODY, SECRET)}) assert verify_signature(signed, BODY, SECRET, prefix="x-acme") From 0a8b3fa854f69b0b1bf84c38d0a23a6780cc3fd4 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 11:53:53 +0100 Subject: [PATCH 02/14] Stop a proxy's X-Request-ID standing in for a Hookdeck event id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The event id fell back to a bare `X-Request-ID` header, which is not a Hookdeck identifier at all: anything in front of the gateway can set it, it is not subject to the configurable `header_prefix`, and it is not unique per delivery — one Hookdeck request fans out to one event per matching connection, so two routes sharing a source produce two events carrying the same request id, and the second would be dropped as a duplicate. It also defeated the branch it was standing in for. `_admit` handles a delivery with no event id deliberately: it warns that the delivery is being processed without deduplication or retry and names `header_prefix` as the likely cause. A substitute id silences that warning while delivering neither guarantee — `POST /events/{id}/retry` with a proxy's id 404s, so the failed run is never handed back. Hookdeck's own `x-hookdeck-requestid` is not a substitute either, for the fan-out reason above; `REQUEST_ID` keeps its place in constants with that written down, so the next person does not reach for it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit 08bc29f44961bc3d76b3307bdbdcac59effef2a2) --- hookdeck/adapter.py | 11 ++++++++--- hookdeck/constants.py | 6 ++++++ tests/test_adapter.py | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/hookdeck/adapter.py b/hookdeck/adapter.py index e35ddb8..b69297e 100644 --- a/hookdeck/adapter.py +++ b/hookdeck/adapter.py @@ -555,9 +555,14 @@ def _parse_delivery( ) -> tuple[Delivery, Optional[web.Response]]: """Build a :class:`Delivery` from a verified request.""" source_name = self._header(request, SOURCE_NAME) - event_id = self._header(request, EVENT_ID) or request.headers.get( - "X-Request-ID", "" - ) + # No fallback. An id that did not come from Hookdeck is worse than + # none: `_admit` has a deliberate branch for a delivery with no event + # id, which warns loudly and names `header_prefix` as the likely cause, + # and a substitute id silences exactly that warning while breaking both + # things the id is for. Dedup keyed on a value Hookdeck did not mint is + # dedup on the wrong thing, and `POST /events/{id}/retry` with it 404s, + # so the failed run is never handed back. + event_id = self._header(request, EVENT_ID) try: attempt = int(self._header(request, ATTEMPT_COUNT) or 0) except ValueError: diff --git a/hookdeck/constants.py b/hookdeck/constants.py index a827258..bb67a4b 100644 --- a/hookdeck/constants.py +++ b/hookdeck/constants.py @@ -17,6 +17,12 @@ SIGNATURE = "signature" SIGNATURE_2 = "signature-2" EVENT_ID = "eventid" +# Hookdeck sends this too, and it is deliberately not used as a delivery +# identity. One request fans out to one event per matching connection, so two +# routes sharing a source produce two events carrying the same request id — +# dedup keyed on it would drop the second as a duplicate. `eventid` is the only +# per-delivery identifier, which is why its absence is treated as "no id" and +# said out loud rather than papered over. REQUEST_ID = "requestid" ATTEMPT_COUNT = "attempt-count" ATTEMPT_TRIGGER = "attempt-trigger" diff --git a/tests/test_adapter.py b/tests/test_adapter.py index 16e02fb..460ae13 100644 --- a/tests/test_adapter.py +++ b/tests/test_adapter.py @@ -909,6 +909,38 @@ async def test_a_delivery_with_no_event_id_is_processed_but_warned_about( assert "without deduplication or retry" in caplog.text +async def test_a_proxys_request_id_is_not_mistaken_for_an_event_id( + client_factory, caplog +): + """`X-Request-ID` is not a Hookdeck identifier and must not stand in. + + Anything in front of the gateway can set it, it is not subject to + `header_prefix`, and one Hookdeck request fans out to one event per + matching connection — so it is not even unique per delivery. Accepting it + would key the ledger on the wrong thing and silence the warning that tells + an operator their `header_prefix` is wrong. + """ + adapter, client = await client_factory({"default": {}}) + seen: list[Any] = [] + adapter.run_agent = lambda event: _record(seen, event) + + raw = json.dumps({"n": 1}).encode() + response = await client.post( + "/hookdeck", + data=raw, + headers={ + "content-type": "application/json", + "x-hookdeck-signature": compute_signature(raw, SECRET), + "X-Request-ID": "req_from_some_proxy", + }, + ) + assert response.status == 202 + await _settle() + assert "without deduplication or retry" in caplog.text + assert adapter._ledger is not None + assert adapter._ledger.get("req_from_some_proxy") is None + + # ---------------------------------------------------------------------- # Deduplication precedence # ---------------------------------------------------------------------- From 42dfbc5d902e0be7cfc4f446c75b8965334fa7c0 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 11:54:54 +0100 Subject: [PATCH 03/14] Clear the sync-ack marker on every terminal path, not just success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_acked_before_completion` records that a sync run outlasted its timeout and was acked 202, so its eventual failure gets the explicit hand-back an async run would get. It was only discarded on success and on a hand-back that worked — so an event that exhausted its retry budget, or whose hand-back was abandoned after three failed API calls, left its id in the set for the life of the process. Small as a leak. The behavioural half matters more: a later sync-mode run of the same event id would find its own id already in the set and be treated as having been acked early when it had not, asking Hookdeck to redeliver an event the 5xx response was already asking it to redeliver. Discard once the hand-back decision has been made, which is the point the marker has done its job, and on the exhausted path that returns before reaching it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit 4ed091c42e6b8f18659c54172717a94ad7817a08) --- hookdeck/adapter.py | 11 +++++++-- tests/test_adapter.py | 53 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/hookdeck/adapter.py b/hookdeck/adapter.py index b69297e..92c1870 100644 --- a/hookdeck/adapter.py +++ b/hookdeck/adapter.py @@ -968,6 +968,7 @@ async def _record_outcome( self._ledger.mark_exhausted( event_id, reason, session_chat_id=session_chat_id ) + self._acked_before_completion.discard(event_id) logger.error( "[hookdeck] Event %s failed %d times — giving up. Inspect it in " "Hookdeck and retry it with `hermes hookdeck retry %s`.", @@ -991,7 +992,14 @@ async def _record_outcome( ) self._ledger.mark_failed(event_id, reason, session_chat_id=session_chat_id) - if not self._should_hand_back(event_id): + hand_back = self._should_hand_back(event_id) + # The marker has done its job the moment that decision is made, whether + # or not the hand-back then succeeds. Leaving it behind on the failure + # path would grow the set for the lifetime of the process, and a later + # sync-mode run of the same event would be treated as having been acked + # early when it was not. + self._acked_before_completion.discard(event_id) + if not hand_back: return if await self._request_redelivery(event_id): logger.info( @@ -1029,7 +1037,6 @@ async def _request_redelivery(self, event_id: str) -> bool: for attempt in range(1, _REDELIVERY_ATTEMPTS + 1): try: await self._api.retry_event(event_id) - self._acked_before_completion.discard(event_id) return True except HookdeckAPIError as exc: last = exc diff --git a/tests/test_adapter.py b/tests/test_adapter.py index 460ae13..7095525 100644 --- a/tests/test_adapter.py +++ b/tests/test_adapter.py @@ -1082,6 +1082,59 @@ async def _slow_failure(_event): assert adapter._api.retried == ["evt_slow_fail"] +async def test_an_abandoned_hand_back_still_clears_its_sync_marker( + client_factory, monkeypatch +): + """The marker goes once the hand-back decision is made, not once it works. + + Keeping it on the failure path grows the set for the life of the process, + and a later sync-mode run of the same id would be treated as having been + acked early when it was not. + """ + from hookdeck.api import HookdeckAPIError + + monkeypatch.setattr("hookdeck.adapter._REDELIVERY_RETRY_INITIAL_SECONDS", 0.0) + adapter, client = await client_factory( + {"default": {}}, ack_mode="sync", sync_timeout_seconds=0.05 + ) + adapter._api.fail_with = HookdeckAPIError(0, "POST", "/events/x/retry", "no route") + gate = asyncio.Event() + + async def _slow_failure(_event): + await gate.wait() + return ProcessingOutcome.FAILURE + + adapter.run_agent = _slow_failure + assert (await post(client, {"n": 1}, event_id="evt_abandoned")).status == 202 + assert "evt_abandoned" in adapter._acked_before_completion + + gate.set() + await _settle() + assert adapter._api.retried == [] + assert adapter._acked_before_completion == set() + + +async def test_an_exhausted_event_clears_its_sync_marker_too(client_factory): + adapter, client = await client_factory( + {"default": {}}, + ack_mode="sync", + sync_timeout_seconds=0.05, + max_agent_retries=0, + ) + gate = asyncio.Event() + + async def _slow_failure(_event): + await gate.wait() + return ProcessingOutcome.FAILURE + + adapter.run_agent = _slow_failure + assert (await post(client, {"n": 1}, event_id="evt_spent")).status == 202 + + gate.set() + await _settle() + assert adapter._acked_before_completion == set() + + async def test_a_sync_failure_within_the_timeout_is_left_to_hookdeck(client_factory): # The 5xx response *is* the retry request there; asking again would double # the redeliveries. From 90c4483e9b862133096a64e0259ed2ff5ef327ee Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 11:57:47 +0100 Subject: [PATCH 04/14] Adopt ruff, and make every blind except say which it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lint only, not formatting. `ruff format` would rewrite nineteen files at once and bury the history for no correctness gain, so `line-length` is set for anyone who chooses to run it and E501 stays off — the long lines here are argparse calls, log statements and comment prose that read worse wrapped. Most of the diff is `ruff --fix`: `Optional[X]` to `X | None`, `Dict` to `dict`, import ordering, `__all__` sorted. All of it is mechanical, and `from __future__ import annotations` is already everywhere, so nothing changes at runtime. The one judgement call is selecting BLE001 rather than ignoring it. This adapter degrades instead of aborting in seven places — a body it cannot read, a skill it cannot load, a CLI whose version it cannot determine, a supervisor that must restart rather than die — and every one is a decision the author made on purpose. Two already carried a `noqa: BLE001` saying so. Selecting the rule makes those two meaningful, gives the other five the same one-line justification, and means the next blind `except` has to be argued for at the point it is written. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit 11201f1d59300d3dbf3be48a632fa7927bc05a75) --- hookdeck/__init__.py | 2 +- hookdeck/adapter.py | 32 ++++++++++++++--------------- hookdeck/api.py | 13 ++++++------ hookdeck/cli.py | 6 +++--- hookdeck/ledger.py | 9 ++++----- hookdeck/provision.py | 21 ++++++++++--------- hookdeck/routing.py | 5 +++-- hookdeck/settings.py | 13 ++++++------ hookdeck/tools.py | 8 ++++---- hookdeck/tunnel.py | 9 ++++----- hookdeck/verify.py | 2 +- pyproject.toml | 47 ++++++++++++++++++++++++++++++++++++++++++- tests/hermes_stub.py | 27 ++++++++++++------------- tests/test_cli.py | 26 ++++++++++++------------ 14 files changed, 133 insertions(+), 87 deletions(-) diff --git a/hookdeck/__init__.py b/hookdeck/__init__.py index ee85a80..d45e233 100644 --- a/hookdeck/__init__.py +++ b/hookdeck/__init__.py @@ -22,7 +22,7 @@ logger = logging.getLogger(__name__) __version__ = "0.1.0" -__all__ = ["register", "PLATFORM_NAME", "__version__"] +__all__ = ["PLATFORM_NAME", "__version__", "register"] PLATFORM_HINT = ( "You were triggered by a webhook delivered through Hookdeck, not by a " diff --git a/hookdeck/adapter.py b/hookdeck/adapter.py index 92c1870..1fdf49a 100644 --- a/hookdeck/adapter.py +++ b/hookdeck/adapter.py @@ -26,7 +26,7 @@ import logging import time from dataclasses import dataclass -from typing import Any, Optional +from typing import Any try: from aiohttp import web @@ -135,8 +135,8 @@ def __init__(self, config: PlatformConfig): self._routes = self.settings.routes self._max_body_bytes = self.settings.max_body_bytes - self._ledger: Optional[RunLedger] = None - self._api: Optional[HookdeckAPI] = None + self._ledger: RunLedger | None = None + self._api: HookdeckAPI | None = None self._tunnels: list[HookdeckTunnel] = [] self._site_runner = None @@ -147,7 +147,7 @@ def __init__(self, config: PlatformConfig): # failures need the same explicit hand-back an async_retry run gets, # because Hookdeck has already recorded the delivery as successful. self._acked_before_completion: set[str] = set() - self._maintenance: Optional[asyncio.Task] = None + self._maintenance: asyncio.Task | None = None self._last_sweep = 0.0 @staticmethod @@ -263,7 +263,7 @@ async def _start_sites(self) -> bool: """Start a listener per address. One family may be absent; both failing is fatal.""" assert self._site_runner is not None started: list[str] = [] - last_error: Optional[OSError] = None + last_error: OSError | None = None for host in self.settings.bind_hosts: try: await web.TCPSite(self._site_runner, host, self._port).start() @@ -513,7 +513,7 @@ async def _handle_delivery(self, request: web.Request) -> web.Response: async def _read_verified_body( self, request: web.Request - ) -> tuple[bytes, Optional[web.Response]]: + ) -> tuple[bytes, web.Response | None]: """Read the body within limits and verify it, before anything parses it.""" too_large = self._respond({"error": "Payload too large"}, status=413) @@ -523,7 +523,7 @@ async def _read_verified_body( raw_body = await request.read() except web.HTTPRequestEntityTooLarge: return b"", too_large - except Exception as exc: + except Exception as exc: # noqa: BLE001 - any read failure is a bad request logger.error("[hookdeck] Failed to read body: %s", exc) return b"", self._respond({"error": "Bad request"}, status=400) if len(raw_body) > self.settings.max_body_bytes: @@ -552,7 +552,7 @@ def _signature_valid(self, request: web.Request, raw_body: bytes) -> bool: def _parse_delivery( self, request: web.Request, raw_body: bytes - ) -> tuple[Delivery, Optional[web.Response]]: + ) -> tuple[Delivery, web.Response | None]: """Build a :class:`Delivery` from a verified request.""" source_name = self._header(request, SOURCE_NAME) # No fallback. An id that did not come from Hookdeck is worse than @@ -630,7 +630,7 @@ def _parse_delivery( async def _reject_if_filtered( self, request: web.Request, delivery: Delivery - ) -> Optional[web.Response]: + ) -> web.Response | None: """Apply the route's own filters. Ignored events answer 200, not an error.""" route = delivery.route @@ -701,7 +701,7 @@ async def _deliver_without_agent(self, delivery: Delivery) -> web.Response: } ) - def _already_handled(self, delivery: Delivery) -> Optional[str]: + def _already_handled(self, delivery: Delivery) -> str | None: """Why this delivery needs no run, if it needs none. Read-only on purpose. Answering this *before* the capacity check is @@ -720,7 +720,7 @@ def _already_handled(self, delivery: Delivery) -> Optional[str]: logger.info("[hookdeck] Skipping %s: %s", delivery.event_id, reason) return reason - def _admit(self, delivery: Delivery) -> Optional[web.Response]: + def _admit(self, delivery: Delivery) -> web.Response | None: """Claim a slot and a ledger entry, or defer. Nothing is recorded for a deferred event: a ledger entry would make @@ -1033,7 +1033,7 @@ async def _request_redelivery(self, event_id: str) -> bool: """ assert self._api is not None delay = _REDELIVERY_RETRY_INITIAL_SECONDS - last: Optional[HookdeckAPIError] = None + last: HookdeckAPIError | None = None for attempt in range(1, _REDELIVERY_ATTEMPTS + 1): try: await self._api.retry_event(event_id) @@ -1066,7 +1066,7 @@ def _respond( body: dict, *, status: int = 200, - headers: Optional[dict] = None, + headers: dict | None = None, ) -> web.Response: """Answer a delivery, refusing any status whose retryability is undeclared. @@ -1172,7 +1172,7 @@ def _apply_skills(self, route: dict, prompt: str) -> str: content = build_skill_invocation_message(command, user_instruction=prompt) if content: return content - except Exception as exc: + except Exception as exc: # noqa: BLE001 - a missing skill must not lose the event logger.warning("[hookdeck] Skill loading failed: %s", exc) return prompt @@ -1222,7 +1222,7 @@ def _sweep_inflight(self, *, force: bool = False) -> None: def _validate_startup(self) -> None: self.settings.validate() - def _bind_hosts(self) -> list[Optional[str]]: + def _bind_hosts(self) -> list[str | None]: return self.settings.bind_hosts def _tunnel_plan(self) -> dict[str, str]: @@ -1270,7 +1270,7 @@ def is_connected(config: PlatformConfig) -> bool: return validate_config(config) -def env_enablement() -> Optional[dict]: +def env_enablement() -> dict | None: """Seed ``PlatformConfig.extra`` from the environment. Lets ``hermes gateway status`` report an env-only setup without diff --git a/hookdeck/api.py b/hookdeck/api.py index 26914c8..b0ade5c 100644 --- a/hookdeck/api.py +++ b/hookdeck/api.py @@ -9,8 +9,9 @@ import asyncio import os +from collections.abc import Mapping from datetime import datetime, timedelta, timezone -from typing import TYPE_CHECKING, Any, Mapping, Optional +from typing import TYPE_CHECKING, Any from .constants import API_BASE_URL @@ -81,11 +82,11 @@ class HookdeckAPI: def __init__( self, - api_key: Optional[str] = None, + api_key: str | None = None, *, base_url: str = API_BASE_URL, timeout: float = 20.0, - client: Optional["httpx.AsyncClient"] = None, + client: httpx.AsyncClient | None = None, ): self.api_key = api_key or os.getenv("HOOKDECK_API_KEY", "") self.base_url = base_url.rstrip("/") @@ -97,7 +98,7 @@ def __init__( # Plumbing # ------------------------------------------------------------------ - def _ensure_client(self) -> "httpx.AsyncClient": + def _ensure_client(self) -> httpx.AsyncClient: if self._client is None: self._client = _httpx().AsyncClient(timeout=self._timeout) return self._client @@ -107,7 +108,7 @@ async def aclose(self) -> None: await self._client.aclose() self._client = None - async def __aenter__(self) -> "HookdeckAPI": + async def __aenter__(self) -> HookdeckAPI: return self async def __aexit__(self, *_exc: Any) -> None: @@ -198,7 +199,7 @@ async def bulk_retry_events(self, query: Mapping[str, Any]) -> Any: # ------------------------------------------------------------------ async def queue_depth( - self, *, hours: int = 24, measures: Optional[list[str]] = None + self, *, hours: int = 24, measures: list[str] | None = None ) -> Any: """GET /metrics/queue-depth over the last *hours*. diff --git a/hookdeck/cli.py b/hookdeck/cli.py index ad6bfb1..2f70f15 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -15,7 +15,7 @@ import subprocess from dataclasses import dataclass from pathlib import Path -from typing import Any, Optional +from typing import Any from .api import HookdeckAPI, HookdeckAPIError, run_sync from .constants import DEFAULT_PATH, DEFAULT_PORT @@ -44,7 +44,7 @@ def _cli_version(binary: str) -> str: out = subprocess.run( [binary, "version"], capture_output=True, text=True, timeout=10 ).stdout - except Exception: + except Exception: # noqa: BLE001 - an unknown version is reported, not raised return "" match = re.search(r"(\d+)\.(\d+)\.(\d+)(?:-([0-9A-Za-z.-]+))?", out) return match.group(0) if match else "" @@ -333,7 +333,7 @@ def _print_local_state(limit: int) -> None: ledger.close() -async def _resolve_connection_id(api: HookdeckAPI, value: str) -> Optional[str]: +async def _resolve_connection_id(api: HookdeckAPI, value: str) -> str | None: if value.startswith("web_") or value.startswith("con_"): return value result = await api.list_connections(name=value) diff --git a/hookdeck/ledger.py b/hookdeck/ledger.py index 6a87271..4d66788 100644 --- a/hookdeck/ledger.py +++ b/hookdeck/ledger.py @@ -33,7 +33,6 @@ import time from dataclasses import dataclass from pathlib import Path -from typing import Optional # Terminal and non-terminal statuses for a delivery. STATUS_RUNNING = "running" @@ -118,7 +117,7 @@ def close(self) -> None: @staticmethod def _admits( - row: Optional[sqlite3.Row], attempt: int, *, operator_initiated: bool = False + row: sqlite3.Row | None, attempt: int, *, operator_initiated: bool = False ) -> tuple[bool, str]: """Whether *attempt* is new work, given what is already recorded. @@ -151,7 +150,7 @@ def _admits( def rejection_reason( self, event_id: str, attempt: int, *, operator_initiated: bool = False - ) -> Optional[str]: + ) -> str | None: """Read-only: why :meth:`admit` would reject this delivery, if it would. Lets a caller recognise a repeat *without* recording anything — which @@ -304,7 +303,7 @@ def record_cancelled( ) self._conn.commit() - def get(self, event_id: str) -> Optional[sqlite3.Row]: + def get(self, event_id: str) -> sqlite3.Row | None: with self._lock: return self._conn.execute( "SELECT * FROM deliveries WHERE event_id = ?", (event_id,) @@ -342,7 +341,7 @@ def cancel_scheduled_resume(self, connection_id: str) -> None: ) self._conn.commit() - def due_resumes(self, now: Optional[float] = None) -> list[sqlite3.Row]: + def due_resumes(self, now: float | None = None) -> list[sqlite3.Row]: moment = time.time() if now is None else now with self._lock: return self._conn.execute( diff --git a/hookdeck/provision.py b/hookdeck/provision.py index f706206..0bd9bf9 100644 --- a/hookdeck/provision.py +++ b/hookdeck/provision.py @@ -15,7 +15,8 @@ from __future__ import annotations -from typing import Any, Mapping, Optional +from collections.abc import Mapping +from typing import Any from .constants import RETRYABLE_STATUSES @@ -54,7 +55,7 @@ def _http_destination_config( url: str, *, - rate_limit: Optional[int], + rate_limit: int | None, rate_limit_period: str, delivery_group_key: str, group_rate: int, @@ -100,16 +101,16 @@ def build_connection_payload( mode: str = "cli", path: str = "/hookdeck", url: str = "", - events: Optional[list[str]] = None, + events: list[str] | None = None, event_path: str = "", - rate_limit: Optional[int] = None, + rate_limit: int | None = None, rate_limit_period: str = "concurrent", delivery_group_key: str = "", group_rate: int = DEFAULT_GROUP_RATE, group_rate_period: str = DEFAULT_GROUP_RATE_PERIOD, retry_count: int = DEFAULT_RETRY_COUNT, retry_interval_ms: int = DEFAULT_RETRY_INTERVAL_MS, - dedupe_window_ms: Optional[int] = DEFAULT_DEDUPE_WINDOW_MS, + dedupe_window_ms: int | None = DEFAULT_DEDUPE_WINDOW_MS, source_secret: str = "", ) -> dict[str, Any]: """Assemble the ``PUT /connections`` body for one Hermes route. @@ -168,7 +169,7 @@ def _destination_spec( mode: str, path: str, url: str, - rate_limit: Optional[int], + rate_limit: int | None, rate_limit_period: str, delivery_group_key: str, group_rate: int, @@ -220,7 +221,7 @@ def _rules( event_path: str, retry_count: int, retry_interval_ms: int, - dedupe_window_ms: Optional[int], + dedupe_window_ms: int | None, ) -> list[dict[str, Any]]: """Connection rules — where most of the reliability actually lives.""" rules: list[dict[str, Any]] = [ @@ -243,7 +244,7 @@ def _rules( def build_event_filter( events: list[str], source_type: str, event_path: str -) -> Optional[dict[str, Any]]: +) -> dict[str, Any] | None: """Turn a route's ``events`` list into a Hookdeck filter rule. Returns ``None`` when the event name's location is unknown — a wrong filter @@ -290,7 +291,7 @@ def retryable_status_codes() -> list[str]: return ordered -def _code_expression_matches(expression: str, status: int) -> Optional[bool]: +def _code_expression_matches(expression: str, status: int) -> bool | None: """Evaluate one Hookdeck retry-rule code expression against *status*. Returns True/False for a positive match, or None when the expression is an @@ -318,7 +319,7 @@ def _code_expression_matches(expression: str, status: int) -> Optional[bool]: def uncovered_statuses( - codes: Optional[list[str]], statuses: tuple[int, ...] = ADAPTER_RETRYABLE_STATUSES + codes: list[str] | None, statuses: tuple[int, ...] = ADAPTER_RETRYABLE_STATUSES ) -> list[int]: """Which of *statuses* a retry rule's ``response_status_codes`` misses. diff --git a/hookdeck/routing.py b/hookdeck/routing.py index 3436f58..652055f 100644 --- a/hookdeck/routing.py +++ b/hookdeck/routing.py @@ -6,7 +6,8 @@ from __future__ import annotations -from typing import Any, Mapping, Optional +from collections.abc import Mapping +from typing import Any from .payload import dig @@ -37,7 +38,7 @@ def route_name_from_path(path_tail: str) -> str: def resolve( routes: Routes, *, path_tail: str = "", source_name: str = "" -) -> tuple[str, Optional[dict]]: +) -> tuple[str, dict | None]: """Pick the route for a delivery, most explicit signal first. Returns the route name and its config, or the name and ``None`` when diff --git a/hookdeck/settings.py b/hookdeck/settings.py index d8c590f..051e537 100644 --- a/hookdeck/settings.py +++ b/hookdeck/settings.py @@ -11,9 +11,10 @@ from __future__ import annotations import os +from collections.abc import Mapping from dataclasses import dataclass, field from pathlib import Path -from typing import Any, Mapping, Optional +from typing import Any from .constants import ( ACK_MODES, @@ -62,7 +63,7 @@ def load_hermes_config() -> dict: return {} -def platform_extra(config: Optional[Mapping[str, Any]] = None) -> dict: +def platform_extra(config: Mapping[str, Any] | None = None) -> dict: """``gateway.platforms.hookdeck.extra`` from a parsed config.""" parsed = load_hermes_config() if config is None else config gateway = parsed.get("gateway") or {} @@ -85,7 +86,7 @@ def configured_state_path() -> Path: LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1", "localhost"}) -def is_loopback(host: Optional[str]) -> bool: +def is_loopback(host: str | None) -> bool: return bool(host) and host in LOOPBACK_HOSTS @@ -97,7 +98,7 @@ class AdapterSettings: # ── Transport ────────────────────────────────────────────────── mode: str = "cli" - host: Optional[str] = None + host: str | None = None port: int = DEFAULT_PORT path: str = DEFAULT_PATH source: str = "" @@ -129,7 +130,7 @@ class AdapterSettings: # ------------------------------------------------------------------ @classmethod - def from_extra(cls, extra: Optional[Mapping[str, Any]]) -> "AdapterSettings": + def from_extra(cls, extra: Mapping[str, Any] | None) -> AdapterSettings: """Build settings from ``platforms.hookdeck.extra``, with env fallbacks. Environment variables are a fallback rather than an override, so a @@ -201,7 +202,7 @@ def verifies_signatures(self) -> bool: return self.signing_secret not in ("", INSECURE_NO_AUTH) @property - def bind_hosts(self) -> list[Optional[str]]: + def bind_hosts(self) -> list[str | None]: """Addresses to listen on. In cli mode that is *both* loopback families. The Hookdeck CLI forwards diff --git a/hookdeck/tools.py b/hookdeck/tools.py index 47099c0..3711d8e 100644 --- a/hookdeck/tools.py +++ b/hookdeck/tools.py @@ -16,7 +16,7 @@ import logging import threading import time -from typing import Any, Optional +from typing import Any from .api import HookdeckAPI, HookdeckAPIError @@ -40,7 +40,7 @@ def _run(coro: Any) -> Any: def _worker() -> None: try: box["value"] = asyncio.run(coro) - except BaseException as exc: + except BaseException as exc: # noqa: BLE001 - re-raised on the caller's thread box["error"] = exc thread = threading.Thread(target=_worker, daemon=True) @@ -115,12 +115,12 @@ def _models(result: Any) -> list[dict]: def _guard(fn: Any) -> Any: """Turn API errors into a message the model can act on.""" - def wrapper(args: Optional[dict] = None, **_: Any) -> str: + def wrapper(args: dict | None = None, **_: Any) -> str: try: return fn(args or {}) except HookdeckAPIError as exc: return f"Hookdeck API error: {exc}" - except Exception as exc: + except Exception as exc: # noqa: BLE001 - the model gets a message, not a traceback return f"Hookdeck tool failed: {exc}" wrapper.__name__ = getattr(fn, "__name__", "hookdeck_tool") diff --git a/hookdeck/tunnel.py b/hookdeck/tunnel.py index 119b508..ca72129 100644 --- a/hookdeck/tunnel.py +++ b/hookdeck/tunnel.py @@ -16,7 +16,6 @@ import logging import os import shutil -from typing import Optional logger = logging.getLogger(__name__) @@ -68,8 +67,8 @@ def __init__( self._api_key = api_key or os.getenv("HOOKDECK_API_KEY", "") self._login_enabled = login self._binary = binary - self._process: Optional[asyncio.subprocess.Process] = None - self._supervisor: Optional[asyncio.Task] = None + self._process: asyncio.subprocess.Process | None = None + self._supervisor: asyncio.Task | None = None self._stopping = False # ------------------------------------------------------------------ @@ -171,7 +170,7 @@ async def _login(self, binary: str) -> None: ) except asyncio.TimeoutError: logger.warning("[hookdeck] `hookdeck ci` timed out after 30s") - except Exception as exc: # pragma: no cover - environment dependent + except Exception as exc: # noqa: BLE001 # pragma: no cover - environment dependent logger.warning("[hookdeck] `hookdeck ci` failed: %s", exc) async def _supervise(self, binary: str) -> None: @@ -182,7 +181,7 @@ async def _supervise(self, binary: str) -> None: await self._run_once(binary) except asyncio.CancelledError: raise - except Exception as exc: + except Exception as exc: # noqa: BLE001 - the supervisor restarts, never dies logger.error("[hookdeck] CLI tunnel error: %s", exc) if self._stopping: diff --git a/hookdeck/verify.py b/hookdeck/verify.py index 91746f6..53a49d5 100644 --- a/hookdeck/verify.py +++ b/hookdeck/verify.py @@ -17,7 +17,7 @@ import base64 import hashlib import hmac -from typing import Iterable, Mapping +from collections.abc import Iterable, Mapping from .constants import DEFAULT_HEADER_PREFIX, SIGNATURE, SIGNATURE_2, header_name diff --git a/pyproject.toml b/pyproject.toml index 1fce3cf..713b454 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,12 +22,57 @@ hookdeck = "hookdeck" # fastapi is provided by the Hermes web server at runtime, not by this # plugin — it is a dev dependency only, so the dashboard routes can be # tested without a Hermes checkout. -dev = ["pytest>=8.0", "pytest-asyncio>=0.23", "fastapi>=0.110"] +dev = ["pytest>=8.0", "pytest-asyncio>=0.23", "fastapi>=0.110", "ruff>=0.16"] [tool.pytest.ini_options] asyncio_mode = "auto" testpaths = ["tests"] +[tool.ruff] +# 88 is what the code was already written to; the handful of lines over it are +# long string literals, which `E501` is told to leave alone below. +line-length = 88 +target-version = "py310" + +[tool.ruff.lint] +select = [ + "E", # pycodestyle + "W", # pycodestyle warnings + "F", # pyflakes — unused imports and names, the ones that are always bugs + "I", # import sorting + "UP", # pyupgrade, bounded by target-version + "B", # bugbear — mutable defaults, `except` that swallows, loop closures + "C4", # comprehensions + "BLE", # blind `except` — see below + "RUF", # ruff's own, mostly correctness +] +ignore = [ + # Off rather than enforced. `line-length` still guides `ruff format` for + # anyone who runs it, but the repo is not formatter-managed: turning E501 + # into an error means either reformatting every file at once, burying the + # history, or a wave of awkward wraps through argparse calls and log + # statements that read worse split. Nothing here is long by accident. + "E501", + # `raise ... from` is used where the chain is informative and deliberately + # omitted where the original exception is noise. + "B904", +] + +# BLE001 is selected on purpose rather than ignored. This adapter degrades +# instead of aborting in a dozen places — a failed API call at boot, a failed +# maintenance tick, a plugin surface that will not register — and every one of +# those is a decision, not an oversight. Selecting the rule means each blind +# `except` carries a `noqa: BLE001` saying which it is, and a new one has to be +# argued for rather than merged unnoticed. + +[tool.ruff.lint.per-file-ignores] +# The `hermes_stub` import in conftest runs for its side effect and must happen +# before anything imports the adapter; `plugin_api` imports its siblings only +# after `_load_plugin_package()`. Both are load-bearing order, and both already +# say so with an inline `noqa: E402` at the import itself, which is where a +# reader needs it. +"tests/*" = ["E402"] + [tool.setuptools] # The manifest, the dashboard bundle and the bundled skill are the plugin as # much as the Python is — a package that ships only the modules installs diff --git a/tests/hermes_stub.py b/tests/hermes_stub.py index 2abd921..476a203 100644 --- a/tests/hermes_stub.py +++ b/tests/hermes_stub.py @@ -24,8 +24,7 @@ from collections import deque from dataclasses import dataclass, field from enum import Enum -from typing import Any, Dict, Optional - +from typing import Any # Names the registry has been told about. Real Hermes mints a Platform member # only for a registered plugin name — "arbitrary strings are rejected to @@ -56,7 +55,7 @@ def _missing_(cls, value): @dataclass class PlatformConfig: enabled: bool = True - extra: Dict[str, Any] = field(default_factory=dict) + extra: dict[str, Any] = field(default_factory=dict) class MessageType(Enum): @@ -73,11 +72,11 @@ class ProcessingOutcome(Enum): class SessionSource: platform: Any = None chat_id: str = "" - chat_name: Optional[str] = None + chat_name: str | None = None chat_type: str = "dm" - user_id: Optional[str] = None - user_name: Optional[str] = None - profile: Optional[str] = None + user_id: str | None = None + user_name: str | None = None + profile: str | None = None @dataclass @@ -86,15 +85,15 @@ class MessageEvent: message_type: Any = MessageType.TEXT source: Any = None raw_message: Any = None - message_id: Optional[str] = None - metadata: Dict[str, Any] = field(default_factory=dict) + message_id: str | None = None + metadata: dict[str, Any] = field(default_factory=dict) @dataclass class SendResult: success: bool - message_id: Optional[str] = None - error: Optional[str] = None + message_id: str | None = None + error: str | None = None class _RouteProcessor: @@ -154,11 +153,11 @@ def __init__(self, config: PlatformConfig): extra = config.extra or {} self._host = extra.get("host") or None self._port = int(extra.get("port", 8787)) - self._routes: Dict[str, dict] = dict(extra.get("routes") or {}) + self._routes: dict[str, dict] = dict(extra.get("routes") or {}) self._max_body_bytes = int(extra.get("max_body_bytes", 1_048_576)) self._route_processor = _RouteProcessor() - self._delivery_info: Dict[str, dict] = {} - self._delivery_info_created: Dict[str, float] = {} + self._delivery_info: dict[str, dict] = {} + self._delivery_info_created: dict[str, float] = {} self._delivery_info_order: deque = deque() self.direct_deliveries: list = [] self.direct_deliver_result = SendResult(success=True) diff --git a/tests/test_cli.py b/tests/test_cli.py index e0b72e6..64b2934 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -6,19 +6,19 @@ def _args(**kwargs) -> argparse.Namespace: - defaults = dict( - route="", - all=False, - source="", - source_type="", - mode="", - url="", - path="", - rate_limit=None, - rate_limit_period="concurrent", - group_key="", - dry_run=True, - ) + defaults = { + "route": "", + "all": False, + "source": "", + "source_type": "", + "mode": "", + "url": "", + "path": "", + "rate_limit": None, + "rate_limit_period": "concurrent", + "group_key": "", + "dry_run": True, + } defaults.update(kwargs) return argparse.Namespace(hookdeck_action="setup", **defaults) From 84eb90b264c0dead671d6ec59cc8add88080d5f3 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 12:00:50 +0100 Subject: [PATCH 05/14] Add CI, a release process, and a guard against upstream drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three jobs on every push and PR: ruff, pytest across 3.10 to 3.13, and a packaging job. The third is the one worth arguing for. This plugin is not just its modules — a wheel missing plugin.yaml, the dashboard bundle or the bundled skill installs perfectly and then registers nothing, and the symptom is a platform that is simply absent with no error to debug. So the job builds the artifact, looks inside it for each of those files, and installs the wheel to confirm the `hermes_agent.plugins` entry point Hermes discovers the plugin by is really declared. Releases are tag-driven and publish to PyPI with Trusted Publishing, so there is no long-lived API token in the repository. The build job refuses to proceed if the tag and `hookdeck.__version__` disagree, and the version now lives only in the module — `pyproject` reads it from there, because two copies drift and the stale one is always the one nobody looks at. Separately, and on a schedule rather than per-PR: the test suite runs entirely against `tests/hermes_stub.py`, which is the only way to exercise ingest without a Hermes checkout but cannot notice the real `WebhookAdapter` renaming something underneath it. The new script parses upstream and asserts every borrowed name is still there — imports, inherited methods, and the `self._…` attributes the base classes own. It is a smoke alarm, not a type check: a signature that changes while the name stays put still gets through, which is why the README points at a real end-to-end run as the thing that proves integration. Verified against the current hermes-agent, and against a copy with two names deliberately moved. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit 5d8bcb221c4b72a99c07de6f159c8d37e0641741) --- .github/workflows/ci.yml | 90 +++++++++++ .github/workflows/release.yml | 90 +++++++++++ .github/workflows/upstream-contract.yml | 37 +++++ CHANGELOG.md | 46 ++++++ pyproject.toml | 8 +- scripts/check_upstream_contract.py | 192 ++++++++++++++++++++++++ 6 files changed, 462 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/ci.yml create mode 100644 .github/workflows/release.yml create mode 100644 .github/workflows/upstream-contract.yml create mode 100644 CHANGELOG.md create mode 100644 scripts/check_upstream_contract.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..ce628aa --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,90 @@ +name: CI + +on: + push: + branches: [main] + pull_request: + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + +jobs: + lint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + - run: pip install -e '.[dev]' + - run: ruff check --output-format=github . + + test: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + # The floor is `requires-python`; the ceiling is whatever is current. + # Hermes decides which of these a real install runs on, so the plugin + # should not be the thing that narrows it. + python-version: ["3.10", "3.11", "3.12", "3.13"] + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: ${{ matrix.python-version }} + - run: pip install -e '.[dev]' + - run: python -m pytest -q + + package: + # A plugin that installs without its manifest, dashboard bundle or skill + # registers nothing, and the failure is silent — the platform simply never + # appears. Building the artifact and looking inside it is the only way that + # gets caught before a release. + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + - run: pip install build twine + - run: python -m build + - run: twine check dist/* + - name: The wheel must carry the non-Python half of the plugin + run: | + set -euo pipefail + wheel=$(ls dist/*.whl) + echo "inspecting $wheel" + contents=$(python -m zipfile --list "$wheel") + for required in \ + hookdeck/plugin.yaml \ + hookdeck/dashboard/manifest.json \ + hookdeck/dashboard/dist/index.js \ + hookdeck/skills/triage-webhook-failures/SKILL.md + do + if ! grep -qF "$required" <<<"$contents"; then + echo "::error::$required is missing from the wheel" + exit 1 + fi + echo " ✓ $required" + done + - name: The entry point Hermes discovers the plugin by must be declared + run: | + set -euo pipefail + pip install dist/*.whl + python - <<'PY' + from importlib.metadata import entry_points + found = entry_points(group="hermes_agent.plugins") + names = {e.name: e.value for e in found} + assert names.get("hookdeck") == "hookdeck", names + print("✓ hermes_agent.plugins entry point:", names) + PY + - uses: actions/upload-artifact@v4 + with: + name: dist + path: dist/ diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..4d7dffe --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,90 @@ +# Tag-driven release to PyPI. +# +# 1. bump `__version__` in hookdeck/__init__.py (pyproject reads it from there) +# 2. update CHANGELOG.md +# 3. git tag v0.2.0 && git push --tags +# +# Publishing uses PyPI Trusted Publishing (OIDC), so there is no API token in +# the repository to leak or rotate. It needs a one-time setup on PyPI: +# Project → Publishing → add a GitHub publisher for hookdeck/hermes-hookdeck, +# workflow `release.yml`, environment `pypi`. +name: Release + +on: + push: + tags: ["v*"] + workflow_dispatch: + +permissions: + contents: read + +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + - run: pip install -e '.[dev]' build twine + + - name: The tag and the package version must agree + if: startsWith(github.ref, 'refs/tags/v') + run: | + set -euo pipefail + tagged="${GITHUB_REF_NAME#v}" + declared=$(python -c 'import hookdeck; print(hookdeck.__version__)') + if [ "$tagged" != "$declared" ]; then + echo "::error::tag $GITHUB_REF_NAME does not match hookdeck.__version__ ($declared)" + exit 1 + fi + echo "✓ releasing $declared" + + # A release that cannot pass its own test suite is not a release. + - run: ruff check . + - run: python -m pytest -q + + - run: python -m build + - run: twine check dist/* + - uses: actions/upload-artifact@v4 + with: + name: dist + path: dist/ + + publish: + needs: build + runs-on: ubuntu-latest + environment: + name: pypi + url: https://pypi.org/p/hermes-hookdeck + permissions: + # The OIDC token pypa/gh-action-pypi-publish exchanges for an upload + # token. Nothing else in this workflow needs it, which is why it is + # scoped to this job rather than the file. + id-token: write + steps: + - uses: actions/download-artifact@v4 + with: + name: dist + path: dist/ + - uses: pypa/gh-action-pypi-publish@release/v1 + + github-release: + needs: publish + runs-on: ubuntu-latest + if: startsWith(github.ref, 'refs/tags/v') + permissions: + contents: write + steps: + - uses: actions/checkout@v4 + - uses: actions/download-artifact@v4 + with: + name: dist + path: dist/ + - name: Publish the GitHub release + env: + GH_TOKEN: ${{ github.token }} + run: | + gh release create "$GITHUB_REF_NAME" dist/* \ + --title "$GITHUB_REF_NAME" \ + --notes "See [CHANGELOG.md](https://github.com/${GITHUB_REPOSITORY}/blob/${GITHUB_REF_NAME}/CHANGELOG.md)." diff --git a/.github/workflows/upstream-contract.yml b/.github/workflows/upstream-contract.yml new file mode 100644 index 0000000..2d75546 --- /dev/null +++ b/.github/workflows/upstream-contract.yml @@ -0,0 +1,37 @@ +# The test suite runs against tests/hermes_stub.py, because the plugin lives +# outside the Hermes tree and there is no other way to exercise the ingest path +# without a Hermes checkout. The blind spot that buys is real: the stub cannot +# notice when the thing it stands in for changes. +# +# So this asks upstream directly, on a schedule rather than on every PR — the +# answer changes when Hermes changes, not when this repo does. A failure here +# is a heads-up, not a broken build. +name: Upstream contract + +on: + schedule: + - cron: "0 6 * * 1" + workflow_dispatch: + push: + paths: + - "scripts/check_upstream_contract.py" + - "tests/hermes_stub.py" + - ".github/workflows/upstream-contract.yml" + +permissions: + contents: read + +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + - name: Fetch the parts of Hermes this plugin borrows from + run: | + git clone --depth 1 --filter=blob:none --sparse \ + https://github.com/NousResearch/hermes-agent.git upstream + git -C upstream sparse-checkout set gateway agent + - run: python scripts/check_upstream_contract.py upstream diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..4df3c86 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,46 @@ +# Changelog + +Notable changes per release. Versions follow [semantic versioning](https://semver.org), +with the caveat that until 1.0 the config surface — `platforms.hookdeck.extra` +— may change in a minor release, and any such change is listed here. + +## Unreleased + +### Fixed + +- A signature header containing a non-ASCII byte answered 500 instead of 401. + The 500 skipped the `EMITTED_STATUS_RETRYABLE` guard and fell inside the + provisioned retry rule's `500-599` range, so Hookdeck retried a forged + request rather than dropping it. +- A delivery's event id no longer falls back to a bare `X-Request-ID` header. + That header is not a Hookdeck identifier, is not subject to `header_prefix`, + and is not unique per delivery — one request fans out to one event per + matching connection. Deliveries genuinely missing `x-hookdeck-eventid` are + processed with the existing warning that dedup and retry are unavailable. +- The marker tracking a `sync` run that outlasted its timeout is now cleared on + every terminal path, not only on success. It previously survived an exhausted + retry budget or an abandoned hand-back, and a later run of the same event id + would be treated as having been acked early when it had not. + +### Added + +- Continuous integration: lint, a test matrix across Python 3.10–3.13, and a + packaging job that asserts the built wheel still carries `plugin.yaml`, the + dashboard bundle and the bundled skill — without which the plugin installs + but registers nothing. +- A weekly check that the Hermes internals this plugin subclasses and calls + still exist upstream, since the test suite otherwise runs entirely against + `tests/hermes_stub.py` and cannot notice the real thing moving. +- Ruff, with `BLE001` selected so each deliberate blind `except` carries its + reason at the point it is written. + +### Changed + +- The package version is read from `hookdeck.__version__` rather than declared + a second time in `pyproject.toml`. + +## 0.1.0 + +First release. Hookdeck platform adapter, `hermes hookdeck` operator commands, +the `hookdeck` agent toolset, the `triage-webhook-failures` skill and the +dashboard tab. diff --git a/pyproject.toml b/pyproject.toml index 713b454..ccb2cff 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,9 @@ [project] name = "hermes-hookdeck" -version = "0.1.0" +# Read from `hookdeck.__version__` rather than repeated here. Two copies drift, +# and the one that drifts is always the one nobody looks at — `hermes plugins` +# reports the module's, PyPI reports this one. +dynamic = ["version"] description = "Hookdeck event gateway plugin for Hermes Agent — verified, queued, retryable webhook triggers" readme = "README.md" requires-python = ">=3.10" @@ -80,6 +83,9 @@ ignore = [ packages = ["hookdeck", "hookdeck.dashboard"] include-package-data = true +[tool.setuptools.dynamic] +version = { attr = "hookdeck.__version__" } + [tool.setuptools.package-data] hookdeck = [ "plugin.yaml", diff --git a/scripts/check_upstream_contract.py b/scripts/check_upstream_contract.py new file mode 100644 index 0000000..e20ab96 --- /dev/null +++ b/scripts/check_upstream_contract.py @@ -0,0 +1,192 @@ +#!/usr/bin/env python3 +"""Check that Hermes still provides everything this plugin borrows from it. + +The plugin lives outside the Hermes tree and its tests run against +``tests/hermes_stub.py``, which is the only way to exercise the ingest path +without a Hermes checkout. The cost of that is a blind spot: the stub cannot +notice when the real ``WebhookAdapter`` renames a method the adapter overrides +or calls, and the first symptom would be a gateway that fails to start. + +So this reads the upstream source and asserts each borrowed name is still +there. It parses rather than imports — importing Hermes would mean installing +its whole dependency tree to answer a question about names. + +It is a smoke alarm, not a type checker. A method that keeps its name and +changes its signature or semantics still gets through, which is why the README +points at a real end-to-end run as the thing that actually proves integration. + + python scripts/check_upstream_contract.py path/to/hermes-agent +""" + +from __future__ import annotations + +import ast +import sys +from pathlib import Path + +#: module path -> names the plugin imports or overrides from it. +#: +#: Sources, in order: the `from gateway...` imports at the top of adapter.py, +#: the attributes hermes_stub.py reproduces, and the lazy `agent.skill_commands` +#: import in `_apply_skills`. +CONTRACT: dict[str, dict[str, list[str]]] = { + "gateway/config.py": { + "module": ["Platform", "PlatformConfig"], + }, + "gateway/platforms/base.py": { + "module": [ + "BasePlatformAdapter", + "MessageEvent", + "MessageType", + "ProcessingOutcome", + "SendResult", + "SessionSource", + ], + # Inherited and called by HookdeckAdapter, or driven by its tests. + "BasePlatformAdapter": [ + "build_source", + "handle_message", + "on_processing_complete", + "_mark_connected", + "_mark_disconnected", + ], + }, + "gateway/platforms/webhook.py": { + "module": ["WebhookAdapter"], + "WebhookAdapter": [ + # Overridden. + "on_processing_complete", + # Called from the delivery path; renaming any of these breaks + # ingest without breaking startup, which is the worse failure. + "_render_prompt", + "_render_delivery_extra", + "_direct_deliver", + "_prune_delivery_info", + ], + }, + "agent/skill_commands.py": { + "module": ["build_skill_invocation_message", "get_skill_commands"], + }, +} + +#: Attributes the adapter reads or writes on `self` that the base classes own. +#: Assigned in `__init__` upstream, so they are found by scanning assignments +#: rather than definitions. +INHERITED_ATTRIBUTES: dict[str, list[str]] = { + "gateway/platforms/webhook.py": [ + "_route_processor", + "_delivery_info", + "_delivery_info_created", + "_delivery_info_order", + ], + "gateway/platforms/base.py": [ + "_background_tasks", + ], +} + + +def _module_level_names(tree: ast.Module) -> set[str]: + names: set[str] = set() + for node in tree.body: + if isinstance(node, (ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)): + names.add(node.name) + elif isinstance(node, ast.Assign): + names.update(t.id for t in node.targets if isinstance(t, ast.Name)) + elif isinstance(node, ast.ImportFrom): + names.update(a.asname or a.name for a in node.names) + return names + + +def _class_member_names(tree: ast.Module, class_name: str) -> set[str]: + for node in ast.walk(tree): + if isinstance(node, ast.ClassDef) and node.name == class_name: + return { + child.name + for child in node.body + if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)) + } + return set() + + +def _self_assigned_names(tree: ast.Module) -> set[str]: + """Every ``self.x = ...`` in the file, which is where base classes declare state.""" + names: set[str] = set() + for node in ast.walk(tree): + targets: list[ast.expr] = [] + if isinstance(node, ast.Assign): + targets = list(node.targets) + elif isinstance(node, ast.AnnAssign): + targets = [node.target] + for target in targets: + if ( + isinstance(target, ast.Attribute) + and isinstance(target.value, ast.Name) + and target.value.id == "self" + ): + names.add(target.attr) + return names + + +def check(root: Path) -> list[str]: + problems: list[str] = [] + + for rel, expectations in CONTRACT.items(): + path = root / rel + if not path.exists(): + problems.append(f"{rel}: module is gone") + continue + tree = ast.parse(path.read_text(encoding="utf-8")) + + for scope, expected in expectations.items(): + found = ( + _module_level_names(tree) + if scope == "module" + else _class_member_names(tree, scope) + ) + if scope != "module" and not found: + problems.append(f"{rel}: class {scope} is gone") + continue + for name in expected: + if name not in found: + where = rel if scope == "module" else f"{rel}:{scope}" + problems.append(f"{where}: {name} is gone") + + for rel, attributes in INHERITED_ATTRIBUTES.items(): + path = root / rel + if not path.exists(): + continue # already reported above + assigned = _self_assigned_names(ast.parse(path.read_text(encoding="utf-8"))) + for attribute in attributes: + if attribute not in assigned: + problems.append(f"{rel}: self.{attribute} is no longer assigned") + + return problems + + +def main() -> int: + if len(sys.argv) != 2: + print(__doc__) + return 2 + root = Path(sys.argv[1]) + if not (root / "gateway").is_dir(): + print(f"! {root} does not look like a hermes-agent checkout") + return 2 + + problems = check(root) + if not problems: + print(f"✓ every name this plugin borrows is still in {root}") + return 0 + + print(f"✗ {len(problems)} name(s) this plugin depends on have moved:\n") + for problem in problems: + print(f" {problem}") + print( + "\nUpdate the adapter and tests/hermes_stub.py together — the stub" + "\nmatching upstream is the only thing making the test suite mean" + "\nanything." + ) + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) From 7771a532c031a03ec21938f41d3d5511f83c2c95 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 13:54:55 +0100 Subject: [PATCH 06/14] Namespace the environment to the Event Gateway, and let a project be pinned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `HOOKDECK_*` becomes `HOOKDECK_EG_*`. Hookdeck's platform is more than one product, and a bare `HOOKDECK_` prefix claims the whole namespace for whichever integration got there first. Every lookup now goes through one helper in constants, which reads the namespaced name and falls back to the old one with a deprecation warning naming its replacement — these names were documented before the plugin was ever published, and a rename should not be why someone's gateway stops booting. The fallback goes at 1.0. `HOOKDECK_API_KEY` is deliberately not renamed the same way. It is the Hookdeck CLI's own documented variable, and this adapter passes it through to the `hookdeck listen` subprocess it spawns — so demanding a second name for one secret would be worse than sharing the ecosystem's convention. `HOOKDECK_EG_API_KEY` wins when set; `HOOKDECK_API_KEY` is a first-class fallback, not a deprecated one. The second half is forward compatibility. A Hookdeck API key is scoped to one project today, so the key implies the project and nothing has to say it. Organisation-level keys that reach several projects are coming, and then it does. `HOOKDECK_EG_PROJECT_ID`, or `project_id` in config.yaml, sends `X-Team-Id` — the same header the Hookdeck CLI sets. Worth setting before it is required, because it closes a hole that only opens with org keys: the dashboard authorises pause and resume by matching connection *names* against the configured routes, so an unscoped organisation key would let a same-named connection in an unrelated project satisfy that check and be paused. `doctor` now reports whether the project is pinned, and says why it does not matter yet. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit 8d1812fba99c89217124f783496aa403d5891195) --- .coverage | Bin 0 -> 53248 bytes CHANGELOG.md | 23 +++++ examples/config.yaml | 2 +- hookdeck/__init__.py | 13 ++- hookdeck/adapter.py | 25 +++-- hookdeck/api.py | 27 +++-- hookdeck/cli.py | 53 +++++++--- hookdeck/constants.py | 92 ++++++++++++++++++ hookdeck/dashboard/plugin_api.py | 4 +- hookdeck/plugin.yaml | 22 +++-- hookdeck/settings.py | 28 ++++-- .../skills/triage-webhook-failures/SKILL.md | 2 +- hookdeck/tunnel.py | 7 +- tests/test_api.py | 55 ++++++++++- tests/test_settings.py | 38 ++++++++ 15 files changed, 333 insertions(+), 58 deletions(-) create mode 100644 .coverage diff --git a/.coverage b/.coverage new file mode 100644 index 0000000000000000000000000000000000000000..a815316a1fbefe01a4736998e46eece171aab4b1 GIT binary patch literal 53248 zcmeI5du$ZP9mi)McYBY$TZ0Q%$E03p;B4>%HxEcE74tB$6=;Ew1gTQa>$|nRe{%NTyHKbLeqCP^UMnowoB;{dK{y?oFwNX?hQrh4)Qbd~+OyYu58@s>R zz4e^|N9e4?3i>U#JG-+p-~G*Jelv4(>s=n+x=AxtxkFDU6jQ#Nlo5s@O|nb~@xvN` z)ma4C;hh5rnWFufcKxJ2`TKzMC=uN|i1c{i4yjJ;6fgUKDz5fE;@5;)Z#|sA1qmPl zB!C2N8v?nlq7bU6VD|NyN_$*2(@IoL+uOkUZOv=9H_O}CuG`cs+i`NWPllzgPF^dg z_1$txP0JlxT$QzCOp7X}mh6Mt*F*wHNKkt{XQX7Ud}wN zT4hqwQTX_$l?EaTMQQB-p{SmYQR~~&N;2A|8nv=ARl=wacJ`QdY14Y5!PMormb8kQ zG&EDwld{^QMl+@wyJ-PyDKuvRc*Ugwtu|VqHalnjT19jUnQxLapFE$r1B^@R4%R|O zdTGdP*__NI+5!1@GKmze9LQl`*wkmrIa6L!&lmQV>*gkPSt#$c>U5CP8nts}lG?6} zI#v0#av^7}Rg9!wE7L-N2Q2lwL-&_61iVu1F?Pk_t5ZbDn7VZwfy$3G!bW^@o<|5( zRWVQRvK)otiL`5`F}35J<}_2$Gq?^tKeOM^Rx;g5Xe)}xWxc~{GBmerR}Oe{ahqvy zU?^$Y&}2QI5GG!Wt8fdU%1UOR*K(tLZRG=DnhQBKd6RC`Ih9Zq zBa?PIl27X-og9T;(rF|4Gf_ zOrZ%}HxS&q`6@PdfG#?6n07#}0c)RbwTG z=DkHX3=M`=I&2vaII(iLOEG9KWQ65*U5~3u(jFTiN2>wX)C}3O;l@#EO+mxxr5c^x zVaq0GaG~A&c4%}?`+}WBLP^H*;}so-ZB^*|Y2&2Vu{#mSD2Jg@8qh?q({x1hXK9b+ z=+~!!2~|xfF}u0bN-0v@-KE|jXOBi36jL>|geub{mNAxN?N$sqnpWXtek`>N17cm* zTV>0%z!|3xwM$bI=044LXcb2q>6XrtJ@l(wu8I*tD^@VMOuhx%om0esZvqL$DQo_a zo#t8Boh&%nx1g7XuSmttMS&MFq=)tuXpd-Dd%KO!NtOSCca16@`bpFpuLqxg=8(fe85&1`oIeX$S;lac75TpbSXJM6Z~Z+!C>1MRAkqctzhB4#qR~hI2_OL^ zfCP{L5jA+UwNJhAuUoP>s4SUEdi~qdOa>c4F{sbGxpjV-1mrmekBh1fI9v5Cq9Usgfo< zaBhIg!vRoPQL0M8uv9$LsU;&yN(0dr5kwc1Dw;~`-5P!T9F*4iLCIUHQZ!DjY4(AJ zRH_C{ytIy9D&w7vjG0L$)i}s(@PJIo)n=IRKs=~CD1b_Nsbw<2``{%vwEow*L8xTF zc6D!bfyTU2jY-1`7Qh&=Wg|Zuos`lW*OeI6;bx-)?|n!?+XS5!7IaE>V|bSa)K(H+ z&H~lNfKtiTRn+L|{eQ{-8j)U=mIb$om!+itHE~<;UGZ@6*`POYRult!0{6~gM~9b2 z0!RP}AOR$R1dsp{KmtgBkZpV}%#=DsDonGBrUFtkp9_>M>CFFw+xgszl2r|KfT+=Pg;up8xwd@;Rwwjj8#+Z&or>^MCKGs?C}Ids_HhdC6ren*R&4 z0&LI!-4F7)c_kZDnE$(GrBj&y^Rv<^%>TJrxpDFQp9PD`Oa7ImsL|v5|8Out0!RP} zAOR$R1dsp{Kmter2_OL^a9b1L7>)$!{eMOpC-A`q2_OL^fCP{L5&1MNimKzdVJE-jQ8cn82M!S4ni3pNL?zu{^ce3Z{eG=^h3&82I1!GO2{q3KR0P(VfQSV5S}=9 z^hEObJH6{yO?+~$|EmGW+7e*NH8#75{O^aG*OTzj!Ds(0Z2yHoNbP>eN0(+N*R$g-gA@Jki8l|g+3a^Q-rOC2 z_S){1gZxkfljV*LTsn35SRZrwy~%T%PxTCgHUleRNfuTDwv)g4SmG(8k=gwJZ%<>% zC|BkK6PEbc?BhQnP2X7DrZbDZ5VOI{9(wDpyPJl;KY2!AtM<#bBHgm2X|)HEq&+Md z`FPtP2wnVecy|ia^z1;}(D8q~^UV`$_A@76Ja!*9y5&66v-~Y-uYM-f<3G)W1<1KY zU@!Q|j%BOK2*2t5kA6J4XXT;H#PP({rR=Kg!jX6Xb@>3d#0?2Iy4k*F(lvjNd$i)$ zV{K2}_2ERgJK$+;YvtX8$35jPNK@_F_vkPAA23(P=4UVUdB^AT5V?$JpJ$lt;mHv% z8NVRpmT>@W-zE--oOY$8!CyT$cJA93|9Ih}Q~z`izX6EOyi9ShO%@-1^sjr@PH@#c zSxm@@$+JgYOxd9mV}p_I=D=@%)3;=(=lmYylLNw$>Q(H&E95gTRWSqpr1IkUI;LmU z*bCRLRt@xBIY=&^9(bjp?Con$&Aalw6DwX9_l6Jo$NS%NH4gIU4-o&y5A+@R04gIJ zBINb^t_dEpm|e(%@%1dbJh*6Nl=}))-NZT1qMv`MoZ-FRx zJp3GB-Tx=nZ#ti8F;E-{AOR$R1dsp{Kmter2_OL^fCP{L5|~2*^!`82|L3p*@X|;C z2_OL^fCP{L5 None: validate_config=validate_config, is_connected=is_connected, env_enablement_fn=env_enablement, - required_env=["HOOKDECK_WEBHOOK_SECRET"], + required_env=[WEBHOOK_SECRET_ENV], install_hint=( "pip install 'aiohttp==3.14.3' httpx # aiohttp is a Hermes extra " "(messaging/slack/…), not a core dependency" ), - allowed_users_env="HOOKDECK_ALLOWED_USERS", - allow_all_env="HOOKDECK_ALLOW_ALL_USERS", + allowed_users_env=ALLOWED_USERS_ENV, + allow_all_env=ALLOW_ALL_USERS_ENV, emoji="🪝", platform_hint=PLATFORM_HINT, # Webhook payloads carry third-party names, emails and phone numbers. diff --git a/hookdeck/adapter.py b/hookdeck/adapter.py index 1fdf49a..d950550 100644 --- a/hookdeck/adapter.py +++ b/hookdeck/adapter.py @@ -52,11 +52,17 @@ ATTEMPT_COUNT, ATTEMPT_TRIGGER, EVENT_ID, + MODE_ENV, OPERATOR_TRIGGERS, + PATH_ENV, PLATFORM_NAME, + PORT_ENV, + SOURCE_ENV, SOURCE_NAME, + WEBHOOK_SECRET_ENV, WILL_RETRY_AFTER, assert_declared_status, + env, header_name, ) from .settings import AdapterSettings @@ -184,7 +190,7 @@ def authorization_is_upstream(self) -> bool: route to the same outcome, and its contract fits: authorization performed by a trusted upstream over an authenticated transport, with no local policy to consult, because a Hookdeck source is not an account - an operator configures in ``HOOKDECK_ALLOWED_USERS``. + an operator configures in ``HOOKDECK_EG_ALLOWED_USERS``. Not a fail-open — false whenever verification is off, so the local allowlist still applies to an ``INSECURE_NO_AUTH`` route. That makes it @@ -1276,18 +1282,17 @@ def env_enablement() -> dict | None: Lets ``hermes gateway status`` report an env-only setup without constructing the adapter. """ - import os - - if not os.getenv("HOOKDECK_WEBHOOK_SECRET"): + secret = env(WEBHOOK_SECRET_ENV) + if not secret: return None - seeded: dict[str, Any] = {"secret": os.getenv("HOOKDECK_WEBHOOK_SECRET", "")} + seeded: dict[str, Any] = {"secret": secret} for env_var, key, cast in ( - ("HOOKDECK_MODE", "mode", str), - ("HOOKDECK_PORT", "port", int), - ("HOOKDECK_PATH", "path", str), - ("HOOKDECK_SOURCE", "source", str), + (MODE_ENV, "mode", str), + (PORT_ENV, "port", int), + (PATH_ENV, "path", str), + (SOURCE_ENV, "source", str), ): - value = os.getenv(env_var) + value = env(env_var) if not value: continue try: diff --git a/hookdeck/api.py b/hookdeck/api.py index b0ade5c..f8e71f8 100644 --- a/hookdeck/api.py +++ b/hookdeck/api.py @@ -8,12 +8,12 @@ from __future__ import annotations import asyncio -import os from collections.abc import Mapping from datetime import datetime, timedelta, timezone from typing import TYPE_CHECKING, Any -from .constants import API_BASE_URL +from .constants import API_BASE_URL, API_KEY_ENV, PROJECT_HEADER, PROJECT_ID_ENV, env +from .constants import api_key as resolve_api_key if TYPE_CHECKING: # pragma: no cover - typing only import httpx @@ -86,9 +86,14 @@ def __init__( *, base_url: str = API_BASE_URL, timeout: float = 20.0, + project_id: str | None = None, client: httpx.AsyncClient | None = None, ): - self.api_key = api_key or os.getenv("HOOKDECK_API_KEY", "") + self.api_key = api_key or resolve_api_key() + # Optional while every Hookdeck API key is scoped to one project, so + # the key implies the project. Organisation-level keys can reach + # several, and then it has to be said. + self.project_id = project_id if project_id is not None else env(PROJECT_ID_ENV) self.base_url = base_url.rstrip("/") self._timeout = timeout self._client = client @@ -124,7 +129,7 @@ async def request( ) -> Any: if not self.api_key: raise HookdeckAPIError( - 401, method, path, "HOOKDECK_API_KEY is not set" + 401, method, path, f"{API_KEY_ENV} is not set" ) client = self._ensure_client() try: @@ -133,10 +138,7 @@ async def request( f"{self.base_url}{path}", json=json, params=_clean_params(params), - headers={ - "Authorization": f"Bearer {self.api_key}", - "Content-Type": "application/json", - }, + headers=self._headers(), ) except Exception as exc: # Timeouts, DNS failures, connection resets. Raised as the same @@ -156,6 +158,15 @@ async def request( except ValueError: return response.text + def _headers(self) -> dict[str, str]: + headers = { + "Authorization": f"Bearer {self.api_key}", + "Content-Type": "application/json", + } + if self.project_id: + headers[PROJECT_HEADER] = self.project_id + return headers + # ------------------------------------------------------------------ # Connections # ------------------------------------------------------------------ diff --git a/hookdeck/cli.py b/hookdeck/cli.py index 2f70f15..f340e93 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -18,7 +18,17 @@ from typing import Any from .api import HookdeckAPI, HookdeckAPIError, run_sync -from .constants import DEFAULT_PATH, DEFAULT_PORT +from .constants import ( + API_KEY_ENV, + CLI_API_KEY_ENV, + DEFAULT_PATH, + DEFAULT_PORT, + MODE_ENV, + PROJECT_ID_ENV, + WEBHOOK_SECRET_ENV, + api_key, + env, +) from .provision import ( build_connection_payload, routes_from_config, @@ -262,7 +272,7 @@ async def _apply() -> int: result = run_sync(_apply()) if result == 0: print( - "\nNext: set HOOKDECK_WEBHOOK_SECRET to your project's signing " + f"\nNext: set {WEBHOOK_SECRET_ENV} to your project's signing " "secret, then start the gateway. Named source types (STRIPE, " "GITHUB, …) still need the provider's own signing secret entered " "on the source in the Hookdeck dashboard." @@ -416,19 +426,40 @@ def render(self) -> None: def _check_credentials(extra: dict) -> list[Check]: + key = api_key() + secret = extra.get("secret") or env(WEBHOOK_SECRET_ENV) + project = extra.get("project_id") or env(PROJECT_ID_ENV) return [ Check( - bool(os.getenv("HOOKDECK_API_KEY")), - "HOOKDECK_API_KEY is set" - if os.getenv("HOOKDECK_API_KEY") - else "HOOKDECK_API_KEY is not set — setup, status and retry will not work", + bool(key), + f"API key is set ({API_KEY_ENV})" + if key + else f"No API key — setup, status and retry will not work. Set " + f"{API_KEY_ENV} (or {CLI_API_KEY_ENV}, which the Hookdeck CLI " + "reads too).", ), Check( - bool(extra.get("secret") or os.getenv("HOOKDECK_WEBHOOK_SECRET")), + bool(secret), "Signing secret is configured" - if (extra.get("secret") or os.getenv("HOOKDECK_WEBHOOK_SECRET")) + if secret else "No signing secret — the adapter will refuse to start. Set " - "HOOKDECK_WEBHOOK_SECRET to your project's signing secret.", + f"{WEBHOOK_SECRET_ENV} to your project's signing secret.", + ), + # Not a failure: a project-scoped key implies its project, and that is + # every key today. It stops being implied once an organisation-level + # key can reach several projects, and an unscoped key would then let + # this gateway act on a same-named connection in the wrong one. + Check( + True, + f"Project pinned to {project}" + if project + else "Project not pinned — fine for a project-scoped API key, " + f"which is every key today. Set {PROJECT_ID_ENV} before moving to " + "an organisation-level key.", + note="" + if project + else "An org-level key reaches several projects, and nothing here " + "would say which one to act on.", ), ] @@ -546,7 +577,7 @@ async def _check_live_connections(routes: dict) -> list[Check]: def _cmd_doctor(_args: argparse.Namespace) -> int: extra = _platform_extra() - mode = extra.get("mode") or os.getenv("HOOKDECK_MODE") or "cli" + mode = extra.get("mode") or env(MODE_ENV) or "cli" routes = routes_from_config(_load_hermes_config()) checks = [*_check_credentials(extra), _check_routes(routes)] @@ -572,7 +603,7 @@ def _cmd_doctor(_args: argparse.Namespace) -> int: ) _report_stranded_runs() - if os.getenv("HOOKDECK_API_KEY"): + if api_key(): print() try: live = run_sync(_check_live_connections(routes)) diff --git a/hookdeck/constants.py b/hookdeck/constants.py index bb67a4b..b1dc29b 100644 --- a/hookdeck/constants.py +++ b/hookdeck/constants.py @@ -8,10 +8,102 @@ from __future__ import annotations +import logging +import os + +logger = logging.getLogger(__name__) + PLATFORM_NAME = "hookdeck" DEFAULT_HEADER_PREFIX = "x-hookdeck" +# ---------------------------------------------------------------------- +# Environment variables +# ---------------------------------------------------------------------- +# +# Namespaced to the Event Gateway. Hookdeck's platform is more than one product +# — Outpost points the other way, at outbound delivery — and a bare `HOOKDECK_` +# prefix claims the whole namespace for whichever integration got there first. +# `HOOKDECK_EG_` says which product the value configures. +ENV_PREFIX = "HOOKDECK_EG_" +#: What these were called before the rename. Still honoured, with a warning. +LEGACY_ENV_PREFIX = "HOOKDECK_" + +WEBHOOK_SECRET_ENV = f"{ENV_PREFIX}WEBHOOK_SECRET" +MODE_ENV = f"{ENV_PREFIX}MODE" +PORT_ENV = f"{ENV_PREFIX}PORT" +PATH_ENV = f"{ENV_PREFIX}PATH" +SOURCE_ENV = f"{ENV_PREFIX}SOURCE" +ALLOWED_USERS_ENV = f"{ENV_PREFIX}ALLOWED_USERS" +ALLOW_ALL_USERS_ENV = f"{ENV_PREFIX}ALLOW_ALL_USERS" + +# The API key is deliberately NOT renamed the same way. `HOOKDECK_API_KEY` is +# the Hookdeck CLI's own documented variable — `hookdeck ci --api-key` defaults +# to it, and this adapter passes it through to the `hookdeck listen` subprocess +# it spawns. Forcing a second name for the same secret would mean setting two +# variables to one value. So the namespaced name wins if present, and the +# ecosystem-wide one is a first-class fallback rather than a deprecated one. +API_KEY_ENV = f"{ENV_PREFIX}API_KEY" +#: Read when the namespaced name is unset, and the name the CLI subprocess is +#: always given, whichever of the two the value came from. +CLI_API_KEY_ENV = "HOOKDECK_API_KEY" + +# Which Hookdeck project the API key should act on. +# +# Optional today and load-bearing soon. A Hookdeck API key is currently scoped +# to one project, so the key implies the project and nothing has to say it. +# Organisation-level keys are coming, and one of those can reach several +# projects — at which point "the project" is no longer implied by the +# credential and has to be stated. +# +# Setting it now is also a safety improvement rather than only future-proofing: +# the dashboard decides which connections this gateway may pause by matching +# *names* against the configured routes, and an unscoped org key would let a +# same-named connection in an unrelated project match. +PROJECT_ID_ENV = f"{ENV_PREFIX}PROJECT_ID" +#: Header that selects the project, sent by the Hookdeck CLI too. Hookdeck's +#: API still calls a project a "team" on the wire; the operator-facing name has +#: been "project" for a while, so the config says project and this says team. +PROJECT_HEADER = "X-Team-Id" + +#: Legacy names already warned about, so a long-running gateway says it once. +_WARNED: set[str] = set() + + +def env(name: str, default: str = "") -> str: + """Read a namespaced adapter variable, honouring its pre-rename name. + + ``HOOKDECK_EG_MODE`` wins; ``HOOKDECK_MODE`` still works and says so once. + The fallback exists because the plugin documented the old names before it + was ever published, and a gateway that refuses to start over a rename is a + worse outcome than a warning. + """ + value = os.getenv(name) + if value: + return value + + if name.startswith(ENV_PREFIX): + legacy = LEGACY_ENV_PREFIX + name[len(ENV_PREFIX) :] + value = os.getenv(legacy) + if value: + if legacy not in _WARNED: + _WARNED.add(legacy) + logger.warning( + "[hookdeck] %s is deprecated — rename it to %s. Hookdeck " + "env vars are namespaced per product now, and the bare " + "HOOKDECK_ prefix is not the Event Gateway's to claim.", + legacy, + name, + ) + return value + + return default + + +def api_key() -> str: + """The Hookdeck API key, namespaced name first, CLI convention second.""" + return os.getenv(API_KEY_ENV) or os.getenv(CLI_API_KEY_ENV, "") + # Suffixes appended to the configured prefix. Hookdeck documents these as # X-Hookdeck-Signature, X-Hookdeck-EventID, X-Hookdeck-Attempt-Count, etc. SIGNATURE = "signature" diff --git a/hookdeck/dashboard/plugin_api.py b/hookdeck/dashboard/plugin_api.py index f7986e5..a1356c4 100644 --- a/hookdeck/dashboard/plugin_api.py +++ b/hookdeck/dashboard/plugin_api.py @@ -17,7 +17,6 @@ import asyncio import importlib.util -import os import sys from pathlib import Path from typing import Any @@ -57,6 +56,7 @@ def _load_plugin_package(): _load_plugin_package() from hookdeck.api import HookdeckAPI, HookdeckAPIError # noqa: E402 +from hookdeck.constants import api_key # noqa: E402 from hookdeck.provision import routes_from_config # noqa: E402 from hookdeck.settings import configured_state_path # noqa: E402 from hookdeck.settings import load_hermes_config as _load_hermes_config # noqa: E402 @@ -118,7 +118,7 @@ def _own_connections(raw: Any) -> tuple[list[dict], int]: async def _hookdeck_state() -> dict: """What Hookdeck still owes this gateway.""" - if not os.getenv("HOOKDECK_API_KEY"): + if not api_key(): return {"configured": False} async with HookdeckAPI() as api: diff --git a/hookdeck/plugin.yaml b/hookdeck/plugin.yaml index 4304f07..39e22ae 100644 --- a/hookdeck/plugin.yaml +++ b/hookdeck/plugin.yaml @@ -18,37 +18,41 @@ author: Hookdeck homepage: https://github.com/hookdeck/hermes-hookdeck requires_env: - - name: HOOKDECK_API_KEY - description: "Hookdeck project API key (Project Settings → Secrets)" + - name: HOOKDECK_EG_API_KEY + description: "Hookdeck API key (Project Settings → Secrets). HOOKDECK_API_KEY is also read — the Hookdeck CLI uses that name too." prompt: "Hookdeck API key" password: true - - name: HOOKDECK_WEBHOOK_SECRET + - name: HOOKDECK_EG_WEBHOOK_SECRET description: "Hookdeck signing secret, used to verify x-hookdeck-signature" prompt: "Hookdeck signing secret" password: true optional_env: - - name: HOOKDECK_MODE + - name: HOOKDECK_EG_MODE description: "Ingestion transport: cli (Hookdeck CLI tunnel) or push (public HTTP). Default: cli" prompt: "Ingestion mode (cli/push)" password: false - - name: HOOKDECK_PORT + - name: HOOKDECK_EG_PORT description: "Port the adapter listens on (default 3579)" prompt: "Listen port" password: false - - name: HOOKDECK_PATH + - name: HOOKDECK_EG_PATH description: "Base path the adapter serves (default /hookdeck)" prompt: "Base path" password: false - - name: HOOKDECK_SOURCE + - name: HOOKDECK_EG_SOURCE description: "Hookdeck source to forward in cli mode. Required unless every route sets its own `source`." prompt: "Hookdeck source name" password: false - - name: HOOKDECK_ALLOWED_USERS + - name: HOOKDECK_EG_PROJECT_ID + description: "Which Hookdeck project the API key acts on. Optional while every key is project-scoped; required once an organisation-level key can reach several." + prompt: "Hookdeck project id" + password: false + - name: HOOKDECK_EG_ALLOWED_USERS description: "Allowlist, only consulted for INSECURE_NO_AUTH routes. Entries are the sender ids the adapter reports: hookdeck:." prompt: "Allowed route names" password: false - - name: HOOKDECK_ALLOW_ALL_USERS + - name: HOOKDECK_EG_ALLOW_ALL_USERS description: "Allow every configured route to trigger the agent (true/false)" prompt: "Allow all routes? (true/false)" password: false diff --git a/hookdeck/settings.py b/hookdeck/settings.py index 051e537..d695501 100644 --- a/hookdeck/settings.py +++ b/hookdeck/settings.py @@ -31,6 +31,13 @@ DEFAULT_RUN_TIMEOUT_SECONDS, DEFAULT_SYNC_TIMEOUT_SECONDS, INSECURE_NO_AUTH, + MODE_ENV, + PATH_ENV, + PORT_ENV, + PROJECT_ID_ENV, + SOURCE_ENV, + WEBHOOK_SECRET_ENV, + env, ) from .routing import tunnel_plan from .ledger import default_state_path @@ -106,6 +113,10 @@ class AdapterSettings: # ── Verification ─────────────────────────────────────────────── signing_secret: str = "" header_prefix: str = DEFAULT_HEADER_PREFIX + #: Which Hookdeck project the API key acts on. Optional while every key is + #: project-scoped; required once an organisation-level key can reach more + #: than one. + project_id: str = "" # ── Run semantics ────────────────────────────────────────────── ack_mode: str = DEFAULT_ACK_MODE @@ -140,10 +151,10 @@ def from_extra(cls, extra: Mapping[str, Any] | None) -> AdapterSettings: """ extra = extra or {} - def text(key: str, env: str = "", default: str = "") -> str: - return str(extra.get(key) or (os.getenv(env) if env else "") or default) + def text(key: str, env_var: str = "", default: str = "") -> str: + return str(extra.get(key) or (env(env_var) if env_var else "") or default) - mode = text("mode", "HOOKDECK_MODE", "cli").lower() + mode = text("mode", MODE_ENV, "cli").lower() return cls( routes=dict(extra.get("routes") or {}), @@ -151,10 +162,11 @@ def text(key: str, env: str = "", default: str = "") -> str: # cli mode is loopback-only by construction: the CLI is the only # thing that should be able to reach the listener. host="127.0.0.1" if mode == "cli" else (extra.get("host") or None), - port=int(extra.get("port") or os.getenv("HOOKDECK_PORT") or DEFAULT_PORT), - path="/" + text("path", "HOOKDECK_PATH", DEFAULT_PATH).strip("/"), - source=text("source", "HOOKDECK_SOURCE"), - signing_secret=text("secret", "HOOKDECK_WEBHOOK_SECRET"), + port=int(extra.get("port") or env(PORT_ENV) or DEFAULT_PORT), + path="/" + text("path", PATH_ENV, DEFAULT_PATH).strip("/"), + source=text("source", SOURCE_ENV), + signing_secret=text("secret", WEBHOOK_SECRET_ENV), + project_id=text("project_id", PROJECT_ID_ENV), header_prefix=text("header_prefix", default=DEFAULT_HEADER_PREFIX), ack_mode=text("ack_mode", default=DEFAULT_ACK_MODE).lower(), max_concurrent=int(extra.get("max_concurrent", DEFAULT_MAX_CONCURRENT)), @@ -237,7 +249,7 @@ def validate(self) -> None: ) if not self.signing_secret: raise ValueError( - "[hookdeck] No signing secret. Set HOOKDECK_WEBHOOK_SECRET (or " + f"[hookdeck] No signing secret. Set {WEBHOOK_SECRET_ENV} (or " "platforms.hookdeck.extra.secret) to the signing secret from " "your Hookdeck project settings. For local testing only, set " f"it to '{INSECURE_NO_AUTH}' while bound to loopback." diff --git a/hookdeck/skills/triage-webhook-failures/SKILL.md b/hookdeck/skills/triage-webhook-failures/SKILL.md index 4e8041b..5e97e9f 100644 --- a/hookdeck/skills/triage-webhook-failures/SKILL.md +++ b/hookdeck/skills/triage-webhook-failures/SKILL.md @@ -22,7 +22,7 @@ Call `hookdeck_list_failed_events`. Group what comes back by `error_code` and - **`503` / `ERR_CONNECTION` in a burst** — the gateway hit its concurrency limit or was down. The events are fine; retrying is the whole fix. - **`401`** — a signing-secret mismatch. Retrying changes nothing until - `HOOKDECK_WEBHOOK_SECRET` matches the project's signing secret. Say so + `HOOKDECK_EG_WEBHOOK_SECRET` matches the project's signing secret. Say so instead of retrying. - **`404`** — no route matched the source. Fix the route config first; the events will keep failing otherwise. diff --git a/hookdeck/tunnel.py b/hookdeck/tunnel.py index ca72129..8e2b402 100644 --- a/hookdeck/tunnel.py +++ b/hookdeck/tunnel.py @@ -17,6 +17,9 @@ import os import shutil +from .constants import CLI_API_KEY_ENV +from .constants import api_key as resolve_api_key + logger = logging.getLogger(__name__) _BACKOFF_INITIAL = 2.0 @@ -64,7 +67,7 @@ def __init__( self._path = path self._source = source self._connection_name = connection_name - self._api_key = api_key or os.getenv("HOOKDECK_API_KEY", "") + self._api_key = api_key or resolve_api_key() self._login_enabled = login self._binary = binary self._process: asyncio.subprocess.Process | None = None @@ -210,7 +213,7 @@ async def _run_once(self, binary: str) -> None: # otherwise exceed asyncio's 64KiB default and raise, bouncing an # otherwise healthy tunnel through the restart backoff. limit=_STDOUT_LINE_LIMIT, - env={**os.environ, **({"HOOKDECK_API_KEY": self._api_key} if self._api_key else {})}, + env={**os.environ, **({CLI_API_KEY_ENV: self._api_key} if self._api_key else {})}, ) assert self._process.stdout is not None async for line in self._process.stdout: diff --git a/tests/test_api.py b/tests/test_api.py index 59272be..eab878d 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -4,6 +4,12 @@ import pytest from hookdeck.api import HookdeckAPI, HookdeckAPIError, _clean_params +from hookdeck.constants import ( + API_KEY_ENV, + CLI_API_KEY_ENV, + PROJECT_HEADER, + PROJECT_ID_ENV, +) def _client(handler) -> httpx.AsyncClient: @@ -50,12 +56,57 @@ def test_clean_params_preserves_repeated_keys(): assert _clean_params([("m", "x"), ("m", "y"), ("n", None)]) == [("m", "x"), ("m", "y")] -async def test_a_missing_api_key_fails_before_any_request(): +async def test_a_missing_api_key_fails_before_any_request(monkeypatch): + # Cleared explicitly: the client falls back to the environment, so a + # developer with a key exported would otherwise not be testing this at all. + monkeypatch.delenv(API_KEY_ENV, raising=False) + monkeypatch.delenv(CLI_API_KEY_ENV, raising=False) api = HookdeckAPI("") - with pytest.raises(HookdeckAPIError, match="HOOKDECK_API_KEY"): + with pytest.raises(HookdeckAPIError, match=API_KEY_ENV): await api.list_events() +async def test_the_cli_api_key_variable_is_a_first_class_fallback(monkeypatch): + # Not deprecated: HOOKDECK_API_KEY is what the Hookdeck CLI itself reads, + # and this adapter passes it to the `hookdeck listen` subprocess. Demanding + # a second name for one secret would be worse than sharing the convention. + monkeypatch.delenv(API_KEY_ENV, raising=False) + monkeypatch.setenv(CLI_API_KEY_ENV, "key_from_the_cli_convention") + assert HookdeckAPI().api_key == "key_from_the_cli_convention" + + +async def test_the_namespaced_api_key_wins(monkeypatch): + monkeypatch.setenv(API_KEY_ENV, "key_namespaced") + monkeypatch.setenv(CLI_API_KEY_ENV, "key_shared") + assert HookdeckAPI().api_key == "key_namespaced" + + +async def test_the_project_is_sent_as_a_header_when_pinned(): + """An org-level key can reach several projects; this says which.""" + seen: dict = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen["headers"] = request.headers + return httpx.Response(200, json={}) + + api = HookdeckAPI("k", project_id="tm_abc", client=_client(handler)) + await api.list_events() + assert seen["headers"][PROJECT_HEADER] == "tm_abc" + + +async def test_no_project_header_when_nothing_is_pinned(monkeypatch): + monkeypatch.delenv(PROJECT_ID_ENV, raising=False) + seen: dict = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen["headers"] = request.headers + return httpx.Response(200, json={}) + + api = HookdeckAPI("k", client=_client(handler)) + await api.list_events() + assert PROJECT_HEADER.lower() not in seen["headers"] + + async def test_non_2xx_carries_the_status_and_body(): def handler(request: httpx.Request) -> httpx.Response: return httpx.Response(422, text='{"data":["measures is required"]}') diff --git a/tests/test_settings.py b/tests/test_settings.py index 969fb3c..ff81a5f 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -2,6 +2,8 @@ import pytest +from hookdeck import constants +from hookdeck.constants import MODE_ENV, PROJECT_ID_ENV, WEBHOOK_SECRET_ENV from hookdeck.settings import AdapterSettings MINIMAL = {"secret": "whsec_x", "routes": {"a": {"source": "s"}}} @@ -112,3 +114,39 @@ 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_the_pre_namespace_env_vars_still_work(monkeypatch, caplog): + """A rename must not be the reason someone's gateway stops booting. + + These names were documented before the plugin was ever published, so a + git-installed gateway may well have them exported. They keep working and + say so once. + """ + monkeypatch.delenv(WEBHOOK_SECRET_ENV, raising=False) + monkeypatch.delenv(MODE_ENV, raising=False) + monkeypatch.setenv("HOOKDECK_WEBHOOK_SECRET", "legacy_secret") + monkeypatch.setenv("HOOKDECK_MODE", "push") + constants._WARNED.clear() + + settings = AdapterSettings.from_extra({}) + assert settings.signing_secret == "legacy_secret" + assert settings.mode == "push" + assert "HOOKDECK_WEBHOOK_SECRET is deprecated" in caplog.text + + +def test_the_namespaced_env_var_wins_over_the_old_one(monkeypatch): + monkeypatch.setenv("HOOKDECK_WEBHOOK_SECRET", "old") + monkeypatch.setenv(WEBHOOK_SECRET_ENV, "new") + assert AdapterSettings.from_extra({}).signing_secret == "new" + + +def test_config_still_outranks_both(monkeypatch): + monkeypatch.setenv(WEBHOOK_SECRET_ENV, "from_env") + assert AdapterSettings.from_extra({"secret": "from_yaml"}).signing_secret == "from_yaml" + + +def test_the_project_can_be_pinned_for_an_org_level_key(monkeypatch): + monkeypatch.setenv(PROJECT_ID_ENV, "tm_from_env") + assert AdapterSettings.from_extra({}).project_id == "tm_from_env" + assert AdapterSettings.from_extra({"project_id": "tm_yaml"}).project_id == "tm_yaml" From c6888fb7e3156f543823963468caab0068c8a398 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 13:59:15 +0100 Subject: [PATCH 07/14] =?UTF-8?q?Drop=20the=20compatibility=20shims=20?= =?UTF-8?q?=E2=80=94=20nothing=20is=20released=20yet?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rename shipped with a fallback that read the old bare `HOOKDECK_` names and warned, plus a changelog entry explaining the migration. Both were written for users who do not exist: the package has never been published, so there is no installed base to keep working and no upgrade to document. So `env()` goes — with no fallback it was pure indirection over `os.getenv` — along with its warning bookkeeping, the two tests pinning the deprecation behaviour, and the migration prose in the README. The `HOOKDECK_EG_` constants stay, and `HOOKDECK_API_KEY` remains a first-class alternative for the API key, which was never about compatibility: it is the Hookdeck CLI's own variable and this adapter hands it to the subprocess it spawns. CHANGELOG.md goes the same way. The release workflow now uses `--generate-notes`, so the commit messages are the changelog rather than a second place to keep the same story in sync. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit e665a72a6f8a29e788e7d81f887f6e62dcc3701e) --- .github/workflows/release.yml | 8 ++-- CHANGELOG.md | 69 ----------------------------------- hookdeck/adapter.py | 7 ++-- hookdeck/api.py | 7 +++- hookdeck/cli.py | 7 ++-- hookdeck/constants.py | 39 -------------------- hookdeck/settings.py | 5 +-- tests/test_adapter.py | 3 +- tests/test_dashboard_api.py | 9 +++-- tests/test_settings.py | 44 ++++++---------------- 10 files changed, 39 insertions(+), 159 deletions(-) delete mode 100644 CHANGELOG.md diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4d7dffe..a402b96 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,8 +1,10 @@ # Tag-driven release to PyPI. # # 1. bump `__version__` in hookdeck/__init__.py (pyproject reads it from there) -# 2. update CHANGELOG.md -# 3. git tag v0.2.0 && git push --tags +# 2. git tag v0.2.0 && git push --tags +# +# Release notes are generated from the commits in the range, so the commit +# messages are the changelog. # # Publishing uses PyPI Trusted Publishing (OIDC), so there is no API token in # the repository to leak or rotate. It needs a one-time setup on PyPI: @@ -87,4 +89,4 @@ jobs: run: | gh release create "$GITHUB_REF_NAME" dist/* \ --title "$GITHUB_REF_NAME" \ - --notes "See [CHANGELOG.md](https://github.com/${GITHUB_REPOSITORY}/blob/${GITHUB_REF_NAME}/CHANGELOG.md)." + --generate-notes diff --git a/CHANGELOG.md b/CHANGELOG.md deleted file mode 100644 index 5f268e0..0000000 --- a/CHANGELOG.md +++ /dev/null @@ -1,69 +0,0 @@ -# Changelog - -Notable changes per release. Versions follow [semantic versioning](https://semver.org), -with the caveat that until 1.0 the config surface — `platforms.hookdeck.extra` -— may change in a minor release, and any such change is listed here. - -## Unreleased - -### Changed — action required - -- **Environment variables are namespaced to the Event Gateway**: `HOOKDECK_*` - becomes `HOOKDECK_EG_*`. Hookdeck's platform is more than one product, and the - bare prefix is not the Event Gateway's to claim. The old names still work and - log a deprecation warning naming their replacement; that fallback goes at 1.0. - - `HOOKDECK_API_KEY` is the deliberate exception. It is the Hookdeck CLI's own - variable, and the adapter passes it to the `hookdeck listen` subprocess it - spawns, so `HOOKDECK_EG_API_KEY` wins when set and `HOOKDECK_API_KEY` remains - a first-class fallback rather than a deprecated one. - -### Added — forward compatibility - -- `HOOKDECK_EG_PROJECT_ID` (or `project_id` in `config.yaml`) pins which - Hookdeck project the API key acts on, sent as `X-Team-Id` — the same header - the Hookdeck CLI uses. Optional while every key is scoped to one project; - required once organisation-level keys can reach several. Also a safety - improvement today: the dashboard authorises pause/resume by matching - connection *names* against configured routes, so an unscoped organisation key - would let a same-named connection in an unrelated project match. - `hermes hookdeck doctor` reports whether the project is pinned. - -### Fixed - -- A signature header containing a non-ASCII byte answered 500 instead of 401. - The 500 skipped the `EMITTED_STATUS_RETRYABLE` guard and fell inside the - provisioned retry rule's `500-599` range, so Hookdeck retried a forged - request rather than dropping it. -- A delivery's event id no longer falls back to a bare `X-Request-ID` header. - That header is not a Hookdeck identifier, is not subject to `header_prefix`, - and is not unique per delivery — one request fans out to one event per - matching connection. Deliveries genuinely missing `x-hookdeck-eventid` are - processed with the existing warning that dedup and retry are unavailable. -- The marker tracking a `sync` run that outlasted its timeout is now cleared on - every terminal path, not only on success. It previously survived an exhausted - retry budget or an abandoned hand-back, and a later run of the same event id - would be treated as having been acked early when it had not. - -### Added - -- Continuous integration: lint, a test matrix across Python 3.10–3.13, and a - packaging job that asserts the built wheel still carries `plugin.yaml`, the - dashboard bundle and the bundled skill — without which the plugin installs - but registers nothing. -- A weekly check that the Hermes internals this plugin subclasses and calls - still exist upstream, since the test suite otherwise runs entirely against - `tests/hermes_stub.py` and cannot notice the real thing moving. -- Ruff, with `BLE001` selected so each deliberate blind `except` carries its - reason at the point it is written. - -### Changed - -- The package version is read from `hookdeck.__version__` rather than declared - a second time in `pyproject.toml`. - -## 0.1.0 - -First release. Hookdeck platform adapter, `hermes hookdeck` operator commands, -the `hookdeck` agent toolset, the `triage-webhook-failures` skill and the -dashboard tab. diff --git a/hookdeck/adapter.py b/hookdeck/adapter.py index d950550..32f847d 100644 --- a/hookdeck/adapter.py +++ b/hookdeck/adapter.py @@ -62,7 +62,6 @@ WEBHOOK_SECRET_ENV, WILL_RETRY_AFTER, assert_declared_status, - env, header_name, ) from .settings import AdapterSettings @@ -1282,7 +1281,9 @@ def env_enablement() -> dict | None: Lets ``hermes gateway status`` report an env-only setup without constructing the adapter. """ - secret = env(WEBHOOK_SECRET_ENV) + import os + + secret = os.getenv(WEBHOOK_SECRET_ENV, "") if not secret: return None seeded: dict[str, Any] = {"secret": secret} @@ -1292,7 +1293,7 @@ def env_enablement() -> dict | None: (PATH_ENV, "path", str), (SOURCE_ENV, "source", str), ): - value = env(env_var) + value = os.getenv(env_var) if not value: continue try: diff --git a/hookdeck/api.py b/hookdeck/api.py index f8e71f8..93171a4 100644 --- a/hookdeck/api.py +++ b/hookdeck/api.py @@ -8,11 +8,12 @@ from __future__ import annotations import asyncio +import os from collections.abc import Mapping from datetime import datetime, timedelta, timezone from typing import TYPE_CHECKING, Any -from .constants import API_BASE_URL, API_KEY_ENV, PROJECT_HEADER, PROJECT_ID_ENV, env +from .constants import API_BASE_URL, API_KEY_ENV, PROJECT_HEADER, PROJECT_ID_ENV from .constants import api_key as resolve_api_key if TYPE_CHECKING: # pragma: no cover - typing only @@ -93,7 +94,9 @@ def __init__( # Optional while every Hookdeck API key is scoped to one project, so # the key implies the project. Organisation-level keys can reach # several, and then it has to be said. - self.project_id = project_id if project_id is not None else env(PROJECT_ID_ENV) + self.project_id = ( + project_id if project_id is not None else os.getenv(PROJECT_ID_ENV, "") + ) self.base_url = base_url.rstrip("/") self._timeout = timeout self._client = client diff --git a/hookdeck/cli.py b/hookdeck/cli.py index f340e93..2dcae6d 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -27,7 +27,6 @@ PROJECT_ID_ENV, WEBHOOK_SECRET_ENV, api_key, - env, ) from .provision import ( build_connection_payload, @@ -427,8 +426,8 @@ def render(self) -> None: def _check_credentials(extra: dict) -> list[Check]: key = api_key() - secret = extra.get("secret") or env(WEBHOOK_SECRET_ENV) - project = extra.get("project_id") or env(PROJECT_ID_ENV) + secret = extra.get("secret") or os.getenv(WEBHOOK_SECRET_ENV) + project = extra.get("project_id") or os.getenv(PROJECT_ID_ENV) return [ Check( bool(key), @@ -577,7 +576,7 @@ async def _check_live_connections(routes: dict) -> list[Check]: def _cmd_doctor(_args: argparse.Namespace) -> int: extra = _platform_extra() - mode = extra.get("mode") or env(MODE_ENV) or "cli" + mode = extra.get("mode") or os.getenv(MODE_ENV) or "cli" routes = routes_from_config(_load_hermes_config()) checks = [*_check_credentials(extra), _check_routes(routes)] diff --git a/hookdeck/constants.py b/hookdeck/constants.py index b1dc29b..5d22baa 100644 --- a/hookdeck/constants.py +++ b/hookdeck/constants.py @@ -8,11 +8,8 @@ from __future__ import annotations -import logging import os -logger = logging.getLogger(__name__) - PLATFORM_NAME = "hookdeck" DEFAULT_HEADER_PREFIX = "x-hookdeck" @@ -26,8 +23,6 @@ # prefix claims the whole namespace for whichever integration got there first. # `HOOKDECK_EG_` says which product the value configures. ENV_PREFIX = "HOOKDECK_EG_" -#: What these were called before the rename. Still honoured, with a warning. -LEGACY_ENV_PREFIX = "HOOKDECK_" WEBHOOK_SECRET_ENV = f"{ENV_PREFIX}WEBHOOK_SECRET" MODE_ENV = f"{ENV_PREFIX}MODE" @@ -66,40 +61,6 @@ #: been "project" for a while, so the config says project and this says team. PROJECT_HEADER = "X-Team-Id" -#: Legacy names already warned about, so a long-running gateway says it once. -_WARNED: set[str] = set() - - -def env(name: str, default: str = "") -> str: - """Read a namespaced adapter variable, honouring its pre-rename name. - - ``HOOKDECK_EG_MODE`` wins; ``HOOKDECK_MODE`` still works and says so once. - The fallback exists because the plugin documented the old names before it - was ever published, and a gateway that refuses to start over a rename is a - worse outcome than a warning. - """ - value = os.getenv(name) - if value: - return value - - if name.startswith(ENV_PREFIX): - legacy = LEGACY_ENV_PREFIX + name[len(ENV_PREFIX) :] - value = os.getenv(legacy) - if value: - if legacy not in _WARNED: - _WARNED.add(legacy) - logger.warning( - "[hookdeck] %s is deprecated — rename it to %s. Hookdeck " - "env vars are namespaced per product now, and the bare " - "HOOKDECK_ prefix is not the Event Gateway's to claim.", - legacy, - name, - ) - return value - - return default - - def api_key() -> str: """The Hookdeck API key, namespaced name first, CLI convention second.""" return os.getenv(API_KEY_ENV) or os.getenv(CLI_API_KEY_ENV, "") diff --git a/hookdeck/settings.py b/hookdeck/settings.py index d695501..8ca3f21 100644 --- a/hookdeck/settings.py +++ b/hookdeck/settings.py @@ -37,7 +37,6 @@ PROJECT_ID_ENV, SOURCE_ENV, WEBHOOK_SECRET_ENV, - env, ) from .routing import tunnel_plan from .ledger import default_state_path @@ -152,7 +151,7 @@ def from_extra(cls, extra: Mapping[str, Any] | None) -> AdapterSettings: extra = extra or {} def text(key: str, env_var: str = "", default: str = "") -> str: - return str(extra.get(key) or (env(env_var) if env_var else "") or default) + return str(extra.get(key) or (os.getenv(env_var, "") if env_var else "") or default) mode = text("mode", MODE_ENV, "cli").lower() @@ -162,7 +161,7 @@ def text(key: str, env_var: str = "", default: str = "") -> str: # cli mode is loopback-only by construction: the CLI is the only # thing that should be able to reach the listener. host="127.0.0.1" if mode == "cli" else (extra.get("host") or None), - port=int(extra.get("port") or env(PORT_ENV) or DEFAULT_PORT), + port=int(extra.get("port") or os.getenv(PORT_ENV) or DEFAULT_PORT), path="/" + text("path", PATH_ENV, DEFAULT_PATH).strip("/"), source=text("source", SOURCE_ENV), signing_secret=text("secret", WEBHOOK_SECRET_ENV), diff --git a/tests/test_adapter.py b/tests/test_adapter.py index 7095525..dbe784c 100644 --- a/tests/test_adapter.py +++ b/tests/test_adapter.py @@ -10,6 +10,7 @@ from aiohttp.test_utils import TestClient, TestServer from hookdeck.adapter import HookdeckAdapter +from hookdeck.constants import WEBHOOK_SECRET_ENV from hookdeck.ledger import RunLedger from hookdeck.verify import compute_signature from tests.hermes_stub import PlatformConfig, ProcessingOutcome, SendResult @@ -461,7 +462,7 @@ def test_startup_refuses_an_unknown_ack_mode(tmp_path): def test_startup_refuses_a_missing_secret(tmp_path, monkeypatch): - monkeypatch.delenv("HOOKDECK_WEBHOOK_SECRET", raising=False) + monkeypatch.delenv(WEBHOOK_SECRET_ENV, raising=False) config = PlatformConfig( extra={"mode": "push", "host": "0.0.0.0", "secret": "", "routes": {"a": {}}} ) diff --git a/tests/test_dashboard_api.py b/tests/test_dashboard_api.py index 5cc30a3..79d20e6 100644 --- a/tests/test_dashboard_api.py +++ b/tests/test_dashboard_api.py @@ -11,6 +11,8 @@ import pytest +from hookdeck.constants import API_KEY_ENV, CLI_API_KEY_ENV + MODULE_PATH = Path(__file__).resolve().parents[1] / "hookdeck" / "dashboard" / "plugin_api.py" @@ -38,7 +40,7 @@ async def test_only_this_gateways_connections_get_controls(plugin_api, monkeypat # The tab renders a Pause button next to every connection it is given. A # project's other connections are unrelated production traffic, so showing # them puts an outage one misclick away. - monkeypatch.setenv("HOOKDECK_API_KEY", "key") + monkeypatch.setenv(API_KEY_ENV, "key") monkeypatch.setattr( plugin_api, "_load_hermes_config", @@ -69,7 +71,7 @@ async def list_connections(self, **_): async def test_the_age_metric_is_not_surfaced(plugin_api, monkeypatch): - monkeypatch.setenv("HOOKDECK_API_KEY", "key") + monkeypatch.setenv(API_KEY_ENV, "key") monkeypatch.setattr(plugin_api, "_load_hermes_config", lambda: {}) class FakeAPI: @@ -90,7 +92,8 @@ async def list_connections(self, **_): return {"models": []} async def test_a_missing_api_key_is_reported_not_raised(plugin_api, monkeypatch): - monkeypatch.delenv("HOOKDECK_API_KEY", raising=False) + monkeypatch.delenv(API_KEY_ENV, raising=False) + monkeypatch.delenv(CLI_API_KEY_ENV, raising=False) result = await plugin_api.overview() assert result["hookdeck"]["configured"] is False # The adapter runs regardless; the tab is observability, not a dependency. diff --git a/tests/test_settings.py b/tests/test_settings.py index ff81a5f..141b13a 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -2,15 +2,20 @@ import pytest -from hookdeck import constants -from hookdeck.constants import MODE_ENV, PROJECT_ID_ENV, WEBHOOK_SECRET_ENV +from hookdeck.constants import ( + MODE_ENV, + PATH_ENV, + PROJECT_ID_ENV, + SOURCE_ENV, + WEBHOOK_SECRET_ENV, +) from hookdeck.settings import AdapterSettings MINIMAL = {"secret": "whsec_x", "routes": {"a": {"source": "s"}}} def test_defaults_are_the_conservative_ones(monkeypatch): - monkeypatch.delenv("HOOKDECK_MODE", raising=False) + monkeypatch.delenv(MODE_ENV, raising=False) settings = AdapterSettings.from_extra(MINIMAL) assert settings.mode == "cli" assert settings.ack_mode == "async_retry" @@ -24,7 +29,7 @@ def test_defaults_are_the_conservative_ones(monkeypatch): def test_config_wins_over_the_environment(monkeypatch): # The other way round would let a stray shell export silently outrank the # file the operator is looking at. - monkeypatch.setenv("HOOKDECK_PATH", "/from-env") + monkeypatch.setenv(PATH_ENV, "/from-env") assert AdapterSettings.from_extra({**MINIMAL, "path": "/from-config"}).path == ( "/from-config" ) @@ -75,14 +80,14 @@ def test_verification_is_off_only_for_the_local_testing_escape_hatch(): ], ) def test_a_configuration_that_cannot_run_is_refused_at_startup(extra, message, monkeypatch): - monkeypatch.delenv("HOOKDECK_WEBHOOK_SECRET", raising=False) - monkeypatch.delenv("HOOKDECK_SOURCE", raising=False) + monkeypatch.delenv(WEBHOOK_SECRET_ENV, raising=False) + monkeypatch.delenv(SOURCE_ENV, raising=False) with pytest.raises(ValueError, match=message): AdapterSettings.from_extra(extra).validate() def test_a_valid_configuration_passes(monkeypatch): - monkeypatch.delenv("HOOKDECK_MODE", raising=False) + monkeypatch.delenv(MODE_ENV, raising=False) AdapterSettings.from_extra(MINIMAL).validate() @@ -116,31 +121,6 @@ def test_the_default_is_used_when_nothing_is_configured(monkeypatch): assert configured_state_path() == default_state_path() -def test_the_pre_namespace_env_vars_still_work(monkeypatch, caplog): - """A rename must not be the reason someone's gateway stops booting. - - These names were documented before the plugin was ever published, so a - git-installed gateway may well have them exported. They keep working and - say so once. - """ - monkeypatch.delenv(WEBHOOK_SECRET_ENV, raising=False) - monkeypatch.delenv(MODE_ENV, raising=False) - monkeypatch.setenv("HOOKDECK_WEBHOOK_SECRET", "legacy_secret") - monkeypatch.setenv("HOOKDECK_MODE", "push") - constants._WARNED.clear() - - settings = AdapterSettings.from_extra({}) - assert settings.signing_secret == "legacy_secret" - assert settings.mode == "push" - assert "HOOKDECK_WEBHOOK_SECRET is deprecated" in caplog.text - - -def test_the_namespaced_env_var_wins_over_the_old_one(monkeypatch): - monkeypatch.setenv("HOOKDECK_WEBHOOK_SECRET", "old") - monkeypatch.setenv(WEBHOOK_SECRET_ENV, "new") - assert AdapterSettings.from_extra({}).signing_secret == "new" - - def test_config_still_outranks_both(monkeypatch): monkeypatch.setenv(WEBHOOK_SECRET_ENV, "from_env") assert AdapterSettings.from_extra({"secret": "from_yaml"}).signing_secret == "from_yaml" From 2ef29e494f398aa0b55de89596b0236e27e91f6d Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 14:08:13 +0100 Subject: [PATCH 08/14] Give the package the metadata a PyPI page needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The build was valid but the page it would produce was bare: no author, no links, no classifiers, and `license = {file = "LICENSE"}` inlining the entire MIT text into a metadata field meant for a short expression. Now a PEP 639 SPDX expression (`License-Expression: MIT`, with the file carried separately), an author, keywords, trove classifiers including the Python versions CI actually tests, and a `[project.urls]` sidebar pointing at the repo, the issue tracker, the Hookdeck docs and Hermes. Also adds the `[build-system]` table the project never had. It was building on whatever the frontend happened to pick, which worked but was unstated, and the SPDX expression above needs setuptools 77 or newer — a build against an older pin would otherwise fail confusingly. Reverts the temporary push trigger from the previous commit. It did its job: CI ran on a real runner and every job passed — lint, the 3.10 to 3.13 matrix, and the packaging job that opens the wheel to check the plugin's manifest, dashboard bundle and skill are inside it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit 5aaf1aebf6fb31b0b2c9a57f9818d5af03f333ff) --- pyproject.toml | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index ccb2cff..414f7e7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,3 +1,10 @@ +# `license = "MIT"` is a PEP 639 SPDX expression, which setuptools only +# understands from 77 — without this pin a build on an older setuptools fails +# confusingly, or silently inlines the whole licence text into the metadata. +[build-system] +requires = ["setuptools>=77"] +build-backend = "setuptools.build_meta" + [project] name = "hermes-hookdeck" # Read from `hookdeck.__version__` rather than repeated here. Two copies drift, @@ -7,7 +14,31 @@ dynamic = ["version"] description = "Hookdeck event gateway plugin for Hermes Agent — verified, queued, retryable webhook triggers" readme = "README.md" requires-python = ">=3.10" -license = { file = "LICENSE" } +license = "MIT" +license-files = ["LICENSE"] +authors = [{ name = "Hookdeck", email = "support@hookdeck.com" }] +keywords = [ + "hookdeck", + "hermes-agent", + "webhooks", + "event-gateway", + "webhook-verification", + "ai-agent", +] +classifiers = [ + "Development Status :: 4 - Beta", + "Intended Audience :: Developers", + "Programming Language :: Python :: 3", + "Programming Language :: Python :: 3.10", + "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12", + "Programming Language :: Python :: 3.13", + "Topic :: Internet :: WWW/HTTP", + "Topic :: Software Development :: Libraries :: Python Modules", + "Topic :: System :: Networking", + "Typing :: Typed", +] + dependencies = [ # Both are already Hermes dependencies; listed so the plugin can be # installed and tested standalone. @@ -15,6 +46,15 @@ dependencies = [ "httpx>=0.27", ] +# The sidebar on the PyPI page. Without these it shows nothing at all, and the +# only route back to the source is whatever the README happens to link. +[project.urls] +Homepage = "https://github.com/hookdeck/hermes-hookdeck" +Repository = "https://github.com/hookdeck/hermes-hookdeck" +Issues = "https://github.com/hookdeck/hermes-hookdeck/issues" +"Hookdeck Event Gateway" = "https://hookdeck.com/docs" +"Hermes Agent" = "https://github.com/NousResearch/hermes-agent" + # Makes `pip install hermes-hookdeck` a working install: Hermes scans this # group and calls `register(ctx)` on whatever the value imports to. Without it # a pip-installed copy sits on disk and is never discovered. From c28ec6f1bee2729da7c514dcf2fe358ff9eafbca Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 14:28:05 +0100 Subject: [PATCH 09/14] Release 0.1.0rc1 as a dry run of the publish path Exercises the real thing rather than a parallel one: the trusted publisher just configured on PyPI, the actual OIDC exchange, the `pypi` environment's reviewer gate, and the artefact CI already builds. A release candidate is the version number you are allowed to spend on finding out the pipeline is wrong, and pip will not install it by default. Also marks pre-release tags as such. `gh release create` would otherwise publish v0.1.0rc1 as a normal release and point the repository's "latest release" at a candidate. (cherry picked from commit 69dc7796cdeef62203194370539e6bde5b0f6aa7) --- .github/workflows/release.yml | 9 ++++++++- hookdeck/__init__.py | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a402b96..0d8e7bc 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -87,6 +87,13 @@ jobs: env: GH_TOKEN: ${{ github.token }} run: | + # v0.1.0rc1 and friends are pre-releases. Saying so keeps them off + # the repository's "latest release", which otherwise points people at + # a release candidate. + prerelease="" + case "$GITHUB_REF_NAME" in + *a[0-9]*|*b[0-9]*|*rc[0-9]*) prerelease="--prerelease" ;; + esac gh release create "$GITHUB_REF_NAME" dist/* \ --title "$GITHUB_REF_NAME" \ - --generate-notes + --generate-notes $prerelease diff --git a/hookdeck/__init__.py b/hookdeck/__init__.py index bb32258..a022b54 100644 --- a/hookdeck/__init__.py +++ b/hookdeck/__init__.py @@ -26,7 +26,7 @@ logger = logging.getLogger(__name__) -__version__ = "0.1.0" +__version__ = "0.1.0rc1" __all__ = ["PLATFORM_NAME", "__version__", "register"] PLATFORM_HINT = ( From 25de5e25bfd47722deb063b585b9a947d1758ac9 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 15:30:27 +0100 Subject: [PATCH 10/14] Give the gateway its own CLI session, and have doctor check the projects agree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first run against a real Hookdeck project failed, and the way it failed is the point: `setup` reported a connection created, the adapter logged that it was listening, and the gateway looked healthy — while `hookdeck listen` restart-looped on "no connection found matching filter" and every event became a CLI_DISCONNECTED ignored event. The cause is that "which project" is two independent settings. The API key decides what `setup`, `status` and the retry hand-back act on; the Hookdeck CLI's own config decides what `hookdeck listen` forwards from. Nothing reconciled them, and the CLI had been left pointing somewhere else weeks earlier. So the adapter now authenticates a CLI config of its own from the API key it already has, beside the ledger, and passes `--hookdeck-config` to every CLI call. Two projects cannot drift apart when only one of them is configurable. `cli_login` is gone: its job is done properly now, and its old implementation ran `hookdeck ci` against the shared config, which switches the CLI's active project for every other use on the machine. `--local` does not avoid that either, despite its help text — hookdeck-cli#332. `doctor` compares the two regardless, because the fallback path still exists: without an API key there is nothing to authenticate with, and the ambient session is all there is. Sessions also identify themselves as `hermes-` rather than inheriting the bare hostname, so a gateway's tunnels are distinguishable from an operator's own `hookdeck listen` in the Hookdeck dashboard. Verified against the live project: 20/20 with the gateway pinning its own session from a directory with no local config to fall back on, and the operator's `~/.config/hookdeck/config.toml` byte-identical afterwards. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit 873b11914c576bf789fc2c98f47be512704d4a44) --- hookdeck/adapter.py | 2 +- hookdeck/api.py | 3 ++ hookdeck/cli.py | 89 +++++++++++++++++++++++++++++++++++++++- hookdeck/settings.py | 23 ++++++++--- hookdeck/tunnel.py | 92 ++++++++++++++++++++++++++++++------------ tests/test_settings.py | 2 +- tests/test_tunnel.py | 37 ++++++++++++++++- 7 files changed, 212 insertions(+), 36 deletions(-) diff --git a/hookdeck/adapter.py b/hookdeck/adapter.py index 32f847d..19aab6b 100644 --- a/hookdeck/adapter.py +++ b/hookdeck/adapter.py @@ -298,7 +298,7 @@ async def _start_tunnels(self) -> bool: source=source, connection_name=route_name, binary=self.settings.cli_binary, - login=self.settings.cli_login, + config_path=self.settings.cli_config_path, ) await tunnel.start() self._tunnels.append(tunnel) diff --git a/hookdeck/api.py b/hookdeck/api.py index 93171a4..d969f0e 100644 --- a/hookdeck/api.py +++ b/hookdeck/api.py @@ -182,6 +182,9 @@ async def upsert_connection(self, payload: Mapping[str, Any]) -> Any: async def list_connections(self, **params: Any) -> Any: return await self.request("GET", "/connections", params=params) + async def list_sources(self, **params: Any) -> Any: + return await self.request("GET", "/sources", params=params) + async def pause_connection(self, connection_id: str) -> Any: return await self.request("PUT", f"/connections/{connection_id}/pause") diff --git a/hookdeck/cli.py b/hookdeck/cli.py index 2dcae6d..daf6b26 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -34,8 +34,13 @@ summarise_payload, uncovered_statuses, ) -from .settings import configured_state_path, load_hermes_config, platform_extra from .ledger import RunLedger +from .settings import ( + configured_state_path, + default_cli_config_path, + load_hermes_config, + platform_extra, +) # ---------------------------------------------------------------------- # Config helpers @@ -472,6 +477,87 @@ def _check_routes(routes: dict) -> Check: ) +def _cli_config_project(path: Path) -> str: + """The project id recorded in a Hookdeck CLI config, if it has one.""" + try: + text = path.read_text() + except OSError: + return "" + match = re.search(r"^\s*project_id\s*=\s*['\"]?([^'\"\s]+)", text, re.M) + return match.group(1) if match else "" + + +def _api_key_project() -> str: + """Which project the API key belongs to, read off anything it can see.""" + async def _go() -> str: + async with HookdeckAPI() as api: + for fetch in (api.list_connections, api.list_sources): + try: + result = await fetch(limit=1) + except (HookdeckAPIError, AttributeError): + continue + models = (result or {}).get("models") or [] + if models and models[0].get("team_id"): + return str(models[0]["team_id"]) + return "" + + try: + return run_sync(_go()) + except Exception: # noqa: BLE001 - a diagnostic must not raise + return "" + + +def _check_cli_project(extra: dict) -> Check: + """The two projects in play must be the same one. + + An API key decides which project `setup`, `status` and the retry hand-back + act on. The Hookdeck CLI's own config decides which project `hookdeck + listen` forwards from. Nothing reconciles them, and when they differ every + visible signal says the gateway is fine: `setup` succeeds, the adapter logs + that it is listening, and only the tunnel's restart loop — "no connection + found matching filter" — says otherwise, while events accumulate as + CLI_DISCONNECTED ignored events. + """ + configured = extra.get("cli_config_path") + owned = Path(configured).expanduser() if configured else default_cli_config_path() + if configured == "": + ambient = Path.home() / ".config" / "hookdeck" / "config.toml" + cli_project = _cli_config_project(ambient) + source = f"your own session ({ambient})" + else: + cli_project = _cli_config_project(owned) + source = f"the gateway's own session ({owned})" + if not cli_project: + return Check( + True, + "The gateway will authenticate its own CLI session on start", + note=f"{owned} does not exist yet; it is created from the API " + "key, so it cannot point at the wrong project.", + ) + + key_project = _api_key_project() + if not key_project: + return Check( + True, + "Could not determine the API key's project — nothing provisioned yet", + note="Re-run doctor after `hermes hookdeck setup`.", + ) + if not cli_project: + return Check(False, f"Could not read a project from {source}") + if cli_project == key_project: + return Check(True, f"CLI and API key agree on project {key_project}") + return Check( + False, + f"Project mismatch: the API key manages {key_project} but the CLI " + f"forwards from {cli_project}", + note="setup provisions one project while `hookdeck listen` forwards " + "from the other. The gateway will look healthy and every event will " + "become a CLI_DISCONNECTED ignored event. Remove " + "platforms.hookdeck.extra.cli_config_path to let the gateway pin its " + "own CLI session from the API key.", + ) + + 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" @@ -582,6 +668,7 @@ def _cmd_doctor(_args: argparse.Namespace) -> int: checks = [*_check_credentials(extra), _check_routes(routes)] if mode == "cli": checks += _check_cli(extra) + checks.append(_check_cli_project(extra)) else: checks.append( Check( diff --git a/hookdeck/settings.py b/hookdeck/settings.py index 8ca3f21..209cd3b 100644 --- a/hookdeck/settings.py +++ b/hookdeck/settings.py @@ -89,6 +89,11 @@ def configured_state_path() -> Path: return Path(configured).expanduser() if configured else default_state_path() +def default_cli_config_path() -> Path: + """Where the gateway keeps its own Hookdeck CLI session.""" + return default_state_path().parent / "cli-config.toml" + + LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1", "localhost"}) @@ -133,7 +138,10 @@ class AdapterSettings: ledger_ttl_seconds: float = DEFAULT_LEDGER_TTL_SECONDS max_body_bytes: int = DEFAULT_MAX_BODY_BYTES cli_binary: str = "hookdeck" - cli_login: bool = False + #: A CLI config the gateway owns, so `hookdeck listen` forwards from the + #: same project the API key manages. Set to "" to use your own ambient + #: `hookdeck login` session instead, and accept that the two can diverge. + cli_config_path: str = "" # ------------------------------------------------------------------ # Construction @@ -198,10 +206,15 @@ def text(key: str, env_var: 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"), - # 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. - cli_login=bool(extra.get("cli_login", False)), + # Beside the ledger, and never the operator's own config: pointing + # `hookdeck ci` at the shared file switches its active project, + # which is not something starting a gateway should do to a tool + # used for other work. + cli_config_path=str( + extra["cli_config_path"] + if "cli_config_path" in extra + else default_cli_config_path() + ), ) # ------------------------------------------------------------------ diff --git a/hookdeck/tunnel.py b/hookdeck/tunnel.py index 8e2b402..ad3125d 100644 --- a/hookdeck/tunnel.py +++ b/hookdeck/tunnel.py @@ -16,6 +16,7 @@ import logging import os import shutil +import socket from .constants import CLI_API_KEY_ENV from .constants import api_key as resolve_api_key @@ -32,6 +33,20 @@ _STDOUT_LINE_LIMIT = 1024 * 1024 +def _device_name() -> str: + """How this gateway's CLI sessions identify themselves to Hookdeck. + + The CLI defaults to the bare hostname, so an operator running their own + `hookdeck listen` on the same machine shows up indistinguishably from the + gateway's. Prefixing says which is which in the dashboard. + """ + try: + host = socket.gethostname() or "unknown" + except OSError: # pragma: no cover - environment dependent + host = "unknown" + return f"hermes-{host}" + + class HookdeckCLIMissing(RuntimeError): """The ``hookdeck`` binary is not on PATH.""" @@ -55,7 +70,7 @@ def __init__( connection_name: str = "", api_key: str = "", binary: str = "hookdeck", - login: bool = False, + config_path: str = "", ): if not source: raise ValueError( @@ -68,7 +83,8 @@ def __init__( self._source = source self._connection_name = connection_name self._api_key = api_key or resolve_api_key() - self._login_enabled = login + #: A CLI config this gateway owns, kept away from the operator's own. + self._config_path = config_path self._binary = binary self._process: asyncio.subprocess.Process | None = None self._supervisor: asyncio.Task | None = None @@ -105,6 +121,12 @@ def listen_args(self) -> list[str]: # immediately when stdout is not a TTY — which it never is here, since # the supervisor pipes it into the gateway log. args += ["--output", "compact"] + if self._config_path: + # The gateway's own CLI session, so `listen` forwards from the same + # project the API key manages rather than whatever the operator's + # shared config happens to point at. + args += ["--hookdeck-config", self._config_path] + args += ["--device-name", _device_name()] return args # ------------------------------------------------------------------ @@ -129,46 +151,64 @@ async def stop(self) -> None: await self._terminate() async def _login(self, binary: str) -> None: - """Non-interactive auth, off by default because it is destructive. - - ``hookdeck ci --api-key`` is not the no-op it looks like. It rewrites - the shared CLI config at ``~/.config/hookdeck/config.toml``: it swaps - the stored key for a CLI session key and switches the CLI's *active - project*. Anyone using the CLI for other work — another ``hookdeck - listen``, a different project — finds their environment silently - repointed by starting a gateway. - - So the gateway does not touch it unless asked. The default path relies - on the operator's existing ``hookdeck login`` and passes - ``HOOKDECK_API_KEY`` through the subprocess environment. + """Point the CLI at the same project the API key manages. + + These are two independent settings, and nothing reconciles them: the + API key decides which project ``setup`` provisions, while the CLI's own + config decides which project ``hookdeck listen`` forwards from. When + they disagree the failure is silent and expensive — ``setup`` succeeds, + the gateway reports itself connected, and the tunnel restart-loops on + "no connection found matching filter" while every event becomes a + ``CLI_DISCONNECTED`` ignored event. + + So the gateway authenticates a CLI config of its own, from the API key + it already has, and passes ``--hookdeck-config`` to every CLI call. + Two projects cannot drift apart when only one of them is configurable. + + This is deliberately not ``hookdeck ci`` against the shared config. + That rewrites ``~/.config/hookdeck/config.toml`` and switches the CLI's + *active project*, so anyone using the CLI for other work would find + their environment repointed by starting a gateway — and it does so even + with ``--local``, which claims to write to the current directory + (observed on CLI 2.4.0). Pointing at our own path avoids the shared + file entirely. + + Without an API key there is nothing to authenticate with, so the + operator's ambient session is used and the mismatch stays possible; + ``hermes hookdeck doctor`` reports it. """ - if not self._login_enabled: + if not self._config_path: return if not self._api_key: - logger.info( - "[hookdeck] cli_login is on but no HOOKDECK_API_KEY is set — " - "relying on an existing `hookdeck login` session" + logger.warning( + "[hookdeck] No API key, so the CLI session cannot be pinned to " + "the same project the adapter manages. Falling back to your " + "own `hookdeck login`. Run `hermes hookdeck doctor` to check " + "the two agree." ) + self._config_path = "" return - logger.warning( - "[hookdeck] cli_login is on: running `hookdeck ci`, which rewrites " - "~/.config/hookdeck/config.toml and switches the CLI's active " - "project. Turn it off if you use the Hookdeck CLI for other work." - ) try: process = await asyncio.create_subprocess_exec( binary, "ci", "--api-key", self._api_key, + "--hookdeck-config", + self._config_path, + "--name", + "hermes-gateway", + "--device-name", + _device_name(), stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, ) stdout, _ = await asyncio.wait_for(process.communicate(), timeout=30) if process.returncode != 0: - logger.warning( - "[hookdeck] `hookdeck ci` exited %s: %s", - process.returncode, + logger.error( + "[hookdeck] Could not authenticate the gateway's CLI " + "config (%s): %s", + self._config_path, (stdout or b"").decode("utf-8", "replace").strip()[:300], ) except asyncio.TimeoutError: diff --git a/tests/test_settings.py b/tests/test_settings.py index 141b13a..08c2b58 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -23,7 +23,7 @@ def test_defaults_are_the_conservative_ones(monkeypatch): # Off by default: cancelling retries discards traffic, and `hookdeck ci` # rewrites the operator's shared CLI config. assert settings.cancel_retries_on_unparseable is False - assert settings.cli_login is False + assert settings.cli_config_path.endswith("cli-config.toml") def test_config_wins_over_the_environment(monkeypatch): diff --git a/tests/test_tunnel.py b/tests/test_tunnel.py index 7252f77..8d9f5e4 100644 --- a/tests/test_tunnel.py +++ b/tests/test_tunnel.py @@ -7,7 +7,9 @@ def test_listen_args_match_the_cli_grammar(): tunnel = HookdeckTunnel(port=3579, path="/hookdeck/github-prs", source="github") - assert tunnel.listen_args() == [ + # The grammar is positional, so the order of the leading arguments is the + # part that matters; flags may be appended after it. + assert tunnel.listen_args()[:7] == [ "listen", "3579", "github", @@ -22,7 +24,7 @@ def test_connection_name_is_passed_as_the_third_positional(): tunnel = HookdeckTunnel( port=3579, path="/hookdeck/x", source="stripe", connection_name="disputes" ) - assert tunnel.listen_args() == [ + assert tunnel.listen_args()[:8] == [ "listen", "3579", "stripe", @@ -59,3 +61,34 @@ def test_output_mode_is_never_the_interactive_default(): # is a pipe — which it always is here, since the supervisor captures it. args = HookdeckTunnel(port=1, path="/x", source="s").listen_args() assert args[args.index("--output") + 1] == "compact" + + +def test_the_gateway_owns_its_cli_session(): + """`listen` must forward from the project the API key manages. + + Those are two independent settings with nothing reconciling them, and the + failure when they differ is silent: setup succeeds, the gateway reports + connected, and the tunnel loops on "no connection found matching filter" + while events pile up as CLI_DISCONNECTED. + """ + tunnel = HookdeckTunnel( + port=3579, path="/hookdeck/x", source="src", + config_path="/tmp/gw-cli-config.toml", + ) + args = tunnel.listen_args() + assert "--hookdeck-config" in args + assert args[args.index("--hookdeck-config") + 1] == "/tmp/gw-cli-config.toml" + + +def test_an_ambient_session_is_still_allowed(): + # Explicitly empty means "use my own `hookdeck login`", accepting the risk. + args = HookdeckTunnel(port=1, path="/p", source="src", config_path="").listen_args() + assert "--hookdeck-config" not in args + + +def test_sessions_are_identifiable_as_this_gateway(): + # The CLI defaults device name to the bare hostname, so the operator's own + # `hookdeck listen` and the gateway's are indistinguishable in Hookdeck. + args = HookdeckTunnel(port=1, path="/p", source="src").listen_args() + assert "--device-name" in args + assert args[args.index("--device-name") + 1].startswith("hermes-") From 8d9b9eea5fb6d29b88d7e45a89fc2b9f3c97da53 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 15:35:08 +0100 Subject: [PATCH 11/14] Ignore the Hookdeck CLI's session directory `hookdeck ci --local` writes credentials to `.hookdeck/config.toml` in the working directory, and says so in its own output. Nothing in this repo creates it, but anyone running that command from the checkout would have a credential file sitting untracked and easy to add by accident. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo (cherry picked from commit 10bd29f9e5f67f49e7685e2a3a43eade243ce75b) --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index b44940f..83008b0 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,6 @@ dist/ # file outside it (see README). *.env .env* +# The Hookdeck CLI writes a session here when run with `--local`, and the +# gateway's own session lives under HERMES_HOME. Both hold credentials. +.hookdeck/ From 8013c26206e649e3c2acc990ecc5d578cf7ad72e Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 16:04:03 +0100 Subject: [PATCH 12/14] Stop tracking .coverage A 53KB binary full of local absolute paths, committed by accident while measuring coverage in the working tree. (cherry picked from commit 895f81693fc0215b514074cbd851fad95bb7891b) --- .coverage | Bin 53248 -> 0 bytes .gitignore | 2 ++ 2 files changed, 2 insertions(+) delete mode 100644 .coverage diff --git a/.coverage b/.coverage deleted file mode 100644 index a815316a1fbefe01a4736998e46eece171aab4b1..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 53248 zcmeI5du$ZP9mi)McYBY$TZ0Q%$E03p;B4>%HxEcE74tB$6=;Ew1gTQa>$|nRe{%NTyHKbLeqCP^UMnowoB;{dK{y?oFwNX?hQrh4)Qbd~+OyYu58@s>R zz4e^|N9e4?3i>U#JG-+p-~G*Jelv4(>s=n+x=AxtxkFDU6jQ#Nlo5s@O|nb~@xvN` z)ma4C;hh5rnWFufcKxJ2`TKzMC=uN|i1c{i4yjJ;6fgUKDz5fE;@5;)Z#|sA1qmPl zB!C2N8v?nlq7bU6VD|NyN_$*2(@IoL+uOkUZOv=9H_O}CuG`cs+i`NWPllzgPF^dg z_1$txP0JlxT$QzCOp7X}mh6Mt*F*wHNKkt{XQX7Ud}wN zT4hqwQTX_$l?EaTMQQB-p{SmYQR~~&N;2A|8nv=ARl=wacJ`QdY14Y5!PMormb8kQ zG&EDwld{^QMl+@wyJ-PyDKuvRc*Ugwtu|VqHalnjT19jUnQxLapFE$r1B^@R4%R|O zdTGdP*__NI+5!1@GKmze9LQl`*wkmrIa6L!&lmQV>*gkPSt#$c>U5CP8nts}lG?6} zI#v0#av^7}Rg9!wE7L-N2Q2lwL-&_61iVu1F?Pk_t5ZbDn7VZwfy$3G!bW^@o<|5( zRWVQRvK)otiL`5`F}35J<}_2$Gq?^tKeOM^Rx;g5Xe)}xWxc~{GBmerR}Oe{ahqvy zU?^$Y&}2QI5GG!Wt8fdU%1UOR*K(tLZRG=DnhQBKd6RC`Ih9Zq zBa?PIl27X-og9T;(rF|4Gf_ zOrZ%}HxS&q`6@PdfG#?6n07#}0c)RbwTG z=DkHX3=M`=I&2vaII(iLOEG9KWQ65*U5~3u(jFTiN2>wX)C}3O;l@#EO+mxxr5c^x zVaq0GaG~A&c4%}?`+}WBLP^H*;}so-ZB^*|Y2&2Vu{#mSD2Jg@8qh?q({x1hXK9b+ z=+~!!2~|xfF}u0bN-0v@-KE|jXOBi36jL>|geub{mNAxN?N$sqnpWXtek`>N17cm* zTV>0%z!|3xwM$bI=044LXcb2q>6XrtJ@l(wu8I*tD^@VMOuhx%om0esZvqL$DQo_a zo#t8Boh&%nx1g7XuSmttMS&MFq=)tuXpd-Dd%KO!NtOSCca16@`bpFpuLqxg=8(fe85&1`oIeX$S;lac75TpbSXJM6Z~Z+!C>1MRAkqctzhB4#qR~hI2_OL^ zfCP{L5jA+UwNJhAuUoP>s4SUEdi~qdOa>c4F{sbGxpjV-1mrmekBh1fI9v5Cq9Usgfo< zaBhIg!vRoPQL0M8uv9$LsU;&yN(0dr5kwc1Dw;~`-5P!T9F*4iLCIUHQZ!DjY4(AJ zRH_C{ytIy9D&w7vjG0L$)i}s(@PJIo)n=IRKs=~CD1b_Nsbw<2``{%vwEow*L8xTF zc6D!bfyTU2jY-1`7Qh&=Wg|Zuos`lW*OeI6;bx-)?|n!?+XS5!7IaE>V|bSa)K(H+ z&H~lNfKtiTRn+L|{eQ{-8j)U=mIb$om!+itHE~<;UGZ@6*`POYRult!0{6~gM~9b2 z0!RP}AOR$R1dsp{KmtgBkZpV}%#=DsDonGBrUFtkp9_>M>CFFw+xgszl2r|KfT+=Pg;up8xwd@;Rwwjj8#+Z&or>^MCKGs?C}Ids_HhdC6ren*R&4 z0&LI!-4F7)c_kZDnE$(GrBj&y^Rv<^%>TJrxpDFQp9PD`Oa7ImsL|v5|8Out0!RP} zAOR$R1dsp{Kmter2_OL^a9b1L7>)$!{eMOpC-A`q2_OL^fCP{L5&1MNimKzdVJE-jQ8cn82M!S4ni3pNL?zu{^ce3Z{eG=^h3&82I1!GO2{q3KR0P(VfQSV5S}=9 z^hEObJH6{yO?+~$|EmGW+7e*NH8#75{O^aG*OTzj!Ds(0Z2yHoNbP>eN0(+N*R$g-gA@Jki8l|g+3a^Q-rOC2 z_S){1gZxkfljV*LTsn35SRZrwy~%T%PxTCgHUleRNfuTDwv)g4SmG(8k=gwJZ%<>% zC|BkK6PEbc?BhQnP2X7DrZbDZ5VOI{9(wDpyPJl;KY2!AtM<#bBHgm2X|)HEq&+Md z`FPtP2wnVecy|ia^z1;}(D8q~^UV`$_A@76Ja!*9y5&66v-~Y-uYM-f<3G)W1<1KY zU@!Q|j%BOK2*2t5kA6J4XXT;H#PP({rR=Kg!jX6Xb@>3d#0?2Iy4k*F(lvjNd$i)$ zV{K2}_2ERgJK$+;YvtX8$35jPNK@_F_vkPAA23(P=4UVUdB^AT5V?$JpJ$lt;mHv% z8NVRpmT>@W-zE--oOY$8!CyT$cJA93|9Ih}Q~z`izX6EOyi9ShO%@-1^sjr@PH@#c zSxm@@$+JgYOxd9mV}p_I=D=@%)3;=(=lmYylLNw$>Q(H&E95gTRWSqpr1IkUI;LmU z*bCRLRt@xBIY=&^9(bjp?Con$&Aalw6DwX9_l6Jo$NS%NH4gIU4-o&y5A+@R04gIJ zBINb^t_dEpm|e(%@%1dbJh*6Nl=}))-NZT1qMv`MoZ-FRx zJp3GB-Tx=nZ#ti8F;E-{AOR$R1dsp{Kmter2_OL^fCP{L5|~2*^!`82|L3p*@X|;C z2_OL^fCP{L5 Date: Tue, 11 Aug 2026 16:14:53 +0100 Subject: [PATCH 13/14] Fix five defects a review of this branch found, and drop project pinning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five things wrong, four of them in code added earlier on this branch. `cli_config_path:` written as a bare YAML key parses as None, and `str()` turned that into the literal "None" — so the adapter would pass `--hookdeck-config None` and write a file by that name into its working directory, while doctor inspected the default path instead. A bare key is the most natural way to write the empty value the docs ask for. `~` was never expanded, and the Hookdeck CLI does not expand it either: an unexpanded path makes the CLI create a directory literally named `~`. doctor did expand it, so the two inspected different files. `hookdeck ci` ran once per route against one shared config file, so with two or more routes a `listen` process read that file while another `ci` was writing it — and one gateway minted a session per route. It now runs once, before any tunnel starts. A failed `hookdeck ci` used to be logged and ignored, leaving `listen` to restart-loop forever against a config that was never written. It now fails the connect, because a tunnel that cannot authenticate is not a tunnel. doctor read the first `project_id` in a CLI config, but those files are multi-section with a top-level `profile` key choosing the active one — so anyone with more than one profile got a mismatch that was not real. It now reads the active profile's section, and distinguishes "no such file" from "file present, names no project", which mean different things. Project pinning goes entirely. It existed to name a project explicitly, and the review found it only ever reached the adapter's own API client — not the agent tools, not the dashboard, not the CLI — so the guarantee it appeared to offer was not one it delivered. Better absent than half-present. The settings test that would have caught two of these had been weakened to `endswith("cli-config.toml")`, which passes for "None" and for an unexpanded `~` path. It now asserts the resolved path, and the six doctor branches that had no coverage at all have tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo --- hookdeck/adapter.py | 23 +++++++- hookdeck/api.py | 15 +---- hookdeck/cli.py | 98 ++++++++++++++++++-------------- hookdeck/constants.py | 17 ------ hookdeck/dashboard/plugin_api.py | 2 +- hookdeck/plugin.yaml | 4 -- hookdeck/settings.py | 32 +++++++---- hookdeck/tools.py | 2 +- hookdeck/tunnel.py | 16 ++++-- tests/test_api.py | 28 --------- tests/test_cli.py | 65 +++++++++++++++++++++ tests/test_settings.py | 38 ++++++++++--- 12 files changed, 205 insertions(+), 135 deletions(-) diff --git a/hookdeck/adapter.py b/hookdeck/adapter.py index 19aab6b..ef2c195 100644 --- a/hookdeck/adapter.py +++ b/hookdeck/adapter.py @@ -64,8 +64,8 @@ assert_declared_status, header_name, ) -from .settings import AdapterSettings from .ledger import RunLedger +from .settings import AdapterSettings from .tunnel import HookdeckCLIMissing, HookdeckTunnel logger = logging.getLogger(__name__) @@ -290,9 +290,16 @@ async def _start_sites(self) -> bool: return True async def _start_tunnels(self) -> bool: + """One CLI session for the gateway, then one `listen` per route. + + Authentication happens once, before any tunnel starts. Doing it per + tunnel would have several `hookdeck ci` processes writing the same + config file while the first `hookdeck listen` is already reading it, + and would mint a session per route for one gateway. + """ try: - for route_name, source in self.settings.tunnels.items(): - tunnel = HookdeckTunnel( + tunnels = [ + HookdeckTunnel( port=self._port, path=f"{self._path}/{route_name}", source=source, @@ -300,6 +307,16 @@ async def _start_tunnels(self) -> bool: binary=self.settings.cli_binary, config_path=self.settings.cli_config_path, ) + for route_name, source in self.settings.tunnels.items() + ] + if tunnels and not await tunnels[0].authenticate(): + logger.error( + "[hookdeck] Refusing to start: the gateway's CLI session " + "could not be authenticated, so `hookdeck listen` would " + "restart-loop against an unusable config." + ) + return False + for tunnel in tunnels: await tunnel.start() self._tunnels.append(tunnel) except HookdeckCLIMissing as exc: diff --git a/hookdeck/api.py b/hookdeck/api.py index d969f0e..968cf2d 100644 --- a/hookdeck/api.py +++ b/hookdeck/api.py @@ -8,12 +8,11 @@ from __future__ import annotations import asyncio -import os from collections.abc import Mapping from datetime import datetime, timedelta, timezone from typing import TYPE_CHECKING, Any -from .constants import API_BASE_URL, API_KEY_ENV, PROJECT_HEADER, PROJECT_ID_ENV +from .constants import API_BASE_URL, API_KEY_ENV from .constants import api_key as resolve_api_key if TYPE_CHECKING: # pragma: no cover - typing only @@ -87,16 +86,9 @@ def __init__( *, base_url: str = API_BASE_URL, timeout: float = 20.0, - project_id: str | None = None, client: httpx.AsyncClient | None = None, ): self.api_key = api_key or resolve_api_key() - # Optional while every Hookdeck API key is scoped to one project, so - # the key implies the project. Organisation-level keys can reach - # several, and then it has to be said. - self.project_id = ( - project_id if project_id is not None else os.getenv(PROJECT_ID_ENV, "") - ) self.base_url = base_url.rstrip("/") self._timeout = timeout self._client = client @@ -162,13 +154,10 @@ async def request( return response.text def _headers(self) -> dict[str, str]: - headers = { + return { "Authorization": f"Bearer {self.api_key}", "Content-Type": "application/json", } - if self.project_id: - headers[PROJECT_HEADER] = self.project_id - return headers # ------------------------------------------------------------------ # Connections diff --git a/hookdeck/cli.py b/hookdeck/cli.py index daf6b26..a730827 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -24,17 +24,16 @@ DEFAULT_PATH, DEFAULT_PORT, MODE_ENV, - PROJECT_ID_ENV, WEBHOOK_SECRET_ENV, api_key, ) +from .ledger import RunLedger from .provision import ( build_connection_payload, routes_from_config, summarise_payload, uncovered_statuses, ) -from .ledger import RunLedger from .settings import ( configured_state_path, default_cli_config_path, @@ -432,7 +431,6 @@ def render(self) -> None: def _check_credentials(extra: dict) -> list[Check]: key = api_key() secret = extra.get("secret") or os.getenv(WEBHOOK_SECRET_ENV) - project = extra.get("project_id") or os.getenv(PROJECT_ID_ENV) return [ Check( bool(key), @@ -449,22 +447,6 @@ def _check_credentials(extra: dict) -> list[Check]: else "No signing secret — the adapter will refuse to start. Set " f"{WEBHOOK_SECRET_ENV} to your project's signing secret.", ), - # Not a failure: a project-scoped key implies its project, and that is - # every key today. It stops being implied once an organisation-level - # key can reach several projects, and an unscoped key would then let - # this gateway act on a same-named connection in the wrong one. - Check( - True, - f"Project pinned to {project}" - if project - else "Project not pinned — fine for a project-scoped API key, " - f"which is every key today. Set {PROJECT_ID_ENV} before moving to " - "an organisation-level key.", - note="" - if project - else "An org-level key reaches several projects, and nothing here " - "would say which one to act on.", - ), ] @@ -477,14 +459,30 @@ def _check_routes(routes: dict) -> Check: ) -def _cli_config_project(path: Path) -> str: - """The project id recorded in a Hookdeck CLI config, if it has one.""" +def _cli_config_project(path: Path) -> tuple[str, bool]: + """The active profile's project id, and whether the file could be read. + + A Hookdeck CLI config is multi-section with a top-level ``profile`` key + selecting the active one, so the first ``project_id`` in the file is not + necessarily the one the CLI will use. Reading the wrong section reports a + mismatch that is not real, which is worse than not checking at all. + + The bool distinguishes "no such file" from "file present, no project in + it" — the caller says something different about each. + """ try: text = path.read_text() except OSError: - return "" - match = re.search(r"^\s*project_id\s*=\s*['\"]?([^'\"\s]+)", text, re.M) - return match.group(1) if match else "" + return "", False + + named = re.search(r"^\s*profile\s*=\s*['\"]?([^'\"\s]+)", text, re.M) + profile = named.group(1) if named else "default" + section = re.search( + rf"^\[{re.escape(profile)}\]\s*$(.*?)(?=^\[|\Z)", text, re.M | re.S + ) + body = section.group(1) if section else text + found = re.search(r"^\s*project_id\s*=\s*['\"]?([^'\"\s]+)", body, re.M) + return (found.group(1) if found else ""), True def _api_key_project() -> str: @@ -519,21 +517,26 @@ def _check_cli_project(extra: dict) -> Check: CLI_DISCONNECTED ignored events. """ configured = extra.get("cli_config_path") - owned = Path(configured).expanduser() if configured else default_cli_config_path() if configured == "": - ambient = Path.home() / ".config" / "hookdeck" / "config.toml" - cli_project = _cli_config_project(ambient) - source = f"your own session ({ambient})" + path = Path.home() / ".config" / "hookdeck" / "config.toml" + source = f"your own session ({path})" else: - cli_project = _cli_config_project(owned) - source = f"the gateway's own session ({owned})" - if not cli_project: - return Check( - True, - "The gateway will authenticate its own CLI session on start", - note=f"{owned} does not exist yet; it is created from the API " - "key, so it cannot point at the wrong project.", - ) + # Matches AdapterSettings: absent or None means the gateway's own. + path = ( + Path(str(configured)).expanduser() + if configured + else default_cli_config_path() + ) + source = f"the gateway's own session ({path})" + + cli_project, readable = _cli_config_project(path) + if configured != "" and not readable: + return Check( + True, + "The gateway will authenticate its own CLI session on start", + note=f"{path} does not exist yet; it is created from the API key, " + "so it cannot point at the wrong project.", + ) key_project = _api_key_project() if not key_project: @@ -543,18 +546,29 @@ def _check_cli_project(extra: dict) -> Check: note="Re-run doctor after `hermes hookdeck setup`.", ) if not cli_project: - return Check(False, f"Could not read a project from {source}") + return Check( + False, + f"No project recorded in {source}", + note="The file exists but names no project for its active profile. " + "Re-authenticate the CLI, or delete the file and let the gateway " + "create it.", + ) if cli_project == key_project: return Check(True, f"CLI and API key agree on project {key_project}") + + fix = ( + "Remove platforms.hookdeck.extra.cli_config_path so the gateway pins " + "its own CLI session from the API key." + if configured == "" + else "Delete the file and let the gateway re-create it from the API key." + ) return Check( False, f"Project mismatch: the API key manages {key_project} but the CLI " f"forwards from {cli_project}", note="setup provisions one project while `hookdeck listen` forwards " - "from the other. The gateway will look healthy and every event will " - "become a CLI_DISCONNECTED ignored event. Remove " - "platforms.hookdeck.extra.cli_config_path to let the gateway pin its " - "own CLI session from the API key.", + f"from the other. The gateway will look healthy and every event will " + f"become a CLI_DISCONNECTED ignored event. {fix}", ) diff --git a/hookdeck/constants.py b/hookdeck/constants.py index 5d22baa..bf045de 100644 --- a/hookdeck/constants.py +++ b/hookdeck/constants.py @@ -43,23 +43,6 @@ #: always given, whichever of the two the value came from. CLI_API_KEY_ENV = "HOOKDECK_API_KEY" -# Which Hookdeck project the API key should act on. -# -# Optional today and load-bearing soon. A Hookdeck API key is currently scoped -# to one project, so the key implies the project and nothing has to say it. -# Organisation-level keys are coming, and one of those can reach several -# projects — at which point "the project" is no longer implied by the -# credential and has to be stated. -# -# Setting it now is also a safety improvement rather than only future-proofing: -# the dashboard decides which connections this gateway may pause by matching -# *names* against the configured routes, and an unscoped org key would let a -# same-named connection in an unrelated project match. -PROJECT_ID_ENV = f"{ENV_PREFIX}PROJECT_ID" -#: Header that selects the project, sent by the Hookdeck CLI too. Hookdeck's -#: API still calls a project a "team" on the wire; the operator-facing name has -#: been "project" for a while, so the config says project and this says team. -PROJECT_HEADER = "X-Team-Id" def api_key() -> str: """The Hookdeck API key, namespaced name first, CLI convention second.""" diff --git a/hookdeck/dashboard/plugin_api.py b/hookdeck/dashboard/plugin_api.py index a1356c4..2adeb63 100644 --- a/hookdeck/dashboard/plugin_api.py +++ b/hookdeck/dashboard/plugin_api.py @@ -57,10 +57,10 @@ def _load_plugin_package(): from hookdeck.api import HookdeckAPI, HookdeckAPIError # noqa: E402 from hookdeck.constants import api_key # noqa: E402 +from hookdeck.ledger import RunLedger # noqa: E402 from hookdeck.provision import routes_from_config # noqa: E402 from hookdeck.settings import configured_state_path # noqa: E402 from hookdeck.settings import load_hermes_config as _load_hermes_config # noqa: E402 -from hookdeck.ledger import RunLedger # noqa: E402 router = APIRouter() diff --git a/hookdeck/plugin.yaml b/hookdeck/plugin.yaml index 39e22ae..efed4a8 100644 --- a/hookdeck/plugin.yaml +++ b/hookdeck/plugin.yaml @@ -44,10 +44,6 @@ optional_env: description: "Hookdeck source to forward in cli mode. Required unless every route sets its own `source`." prompt: "Hookdeck source name" password: false - - name: HOOKDECK_EG_PROJECT_ID - description: "Which Hookdeck project the API key acts on. Optional while every key is project-scoped; required once an organisation-level key can reach several." - prompt: "Hookdeck project id" - password: false - name: HOOKDECK_EG_ALLOWED_USERS description: "Allowlist, only consulted for INSECURE_NO_AUTH routes. Entries are the sender ids the adapter reports: hookdeck:." prompt: "Allowed route names" diff --git a/hookdeck/settings.py b/hookdeck/settings.py index 209cd3b..1fd565b 100644 --- a/hookdeck/settings.py +++ b/hookdeck/settings.py @@ -34,12 +34,11 @@ MODE_ENV, PATH_ENV, PORT_ENV, - PROJECT_ID_ENV, SOURCE_ENV, WEBHOOK_SECRET_ENV, ) -from .routing import tunnel_plan from .ledger import default_state_path +from .routing import tunnel_plan MODES = ("cli", "push") @@ -89,6 +88,20 @@ def configured_state_path() -> Path: return Path(configured).expanduser() if configured else default_state_path() +def _cli_config_path(extra: Mapping[str, Any]) -> str: + """Resolve `cli_config_path`, expanding `~` and treating None as unset. + + `~` matters: the Hookdeck CLI does not expand it either, so an unexpanded + path makes the CLI create a directory literally named `~` in the working + directory — and `doctor` would inspect a different file than the adapter + writes. + """ + if "cli_config_path" not in extra or extra["cli_config_path"] is None: + return str(default_cli_config_path()) + configured = str(extra["cli_config_path"]) + return str(Path(configured).expanduser()) if configured else "" + + def default_cli_config_path() -> Path: """Where the gateway keeps its own Hookdeck CLI session.""" return default_state_path().parent / "cli-config.toml" @@ -117,10 +130,6 @@ class AdapterSettings: # ── Verification ─────────────────────────────────────────────── signing_secret: str = "" header_prefix: str = DEFAULT_HEADER_PREFIX - #: Which Hookdeck project the API key acts on. Optional while every key is - #: project-scoped; required once an organisation-level key can reach more - #: than one. - project_id: str = "" # ── Run semantics ────────────────────────────────────────────── ack_mode: str = DEFAULT_ACK_MODE @@ -173,7 +182,6 @@ def text(key: str, env_var: str = "", default: str = "") -> str: path="/" + text("path", PATH_ENV, DEFAULT_PATH).strip("/"), source=text("source", SOURCE_ENV), signing_secret=text("secret", WEBHOOK_SECRET_ENV), - project_id=text("project_id", PROJECT_ID_ENV), header_prefix=text("header_prefix", default=DEFAULT_HEADER_PREFIX), ack_mode=text("ack_mode", default=DEFAULT_ACK_MODE).lower(), max_concurrent=int(extra.get("max_concurrent", DEFAULT_MAX_CONCURRENT)), @@ -210,11 +218,11 @@ def text(key: str, env_var: str = "", default: str = "") -> str: # `hookdeck ci` at the shared file switches its active project, # which is not something starting a gateway should do to a tool # used for other work. - cli_config_path=str( - extra["cli_config_path"] - if "cli_config_path" in extra - else default_cli_config_path() - ), + # `cli_config_path:` with no value parses as None, which `str()` + # would turn into the literal "None" and write a file by that name + # into the gateway's cwd. An explicit empty string is different and + # must survive: it means "use my own ambient session". + cli_config_path=_cli_config_path(extra), ) # ------------------------------------------------------------------ diff --git a/hookdeck/tools.py b/hookdeck/tools.py index 3711d8e..3ab417f 100644 --- a/hookdeck/tools.py +++ b/hookdeck/tools.py @@ -85,8 +85,8 @@ def _cancel_scheduled_resume(connection_id: str) -> None: def _with_ledger(action) -> None: - from .settings import configured_state_path from .ledger import RunLedger + from .settings import configured_state_path # The path the adapter actually reads, honouring a configured state_path. # Writing a pause deadline anywhere else records it where nothing will diff --git a/hookdeck/tunnel.py b/hookdeck/tunnel.py index ad3125d..cc10cb4 100644 --- a/hookdeck/tunnel.py +++ b/hookdeck/tunnel.py @@ -135,7 +135,6 @@ def listen_args(self) -> list[str]: async def start(self) -> None: binary = self.resolve_binary() - await self._login(binary) self._stopping = False self._supervisor = asyncio.create_task(self._supervise(binary)) @@ -150,7 +149,7 @@ async def stop(self) -> None: self._supervisor = None await self._terminate() - async def _login(self, binary: str) -> None: + async def authenticate(self) -> bool: """Point the CLI at the same project the API key manages. These are two independent settings, and nothing reconciles them: the @@ -178,7 +177,8 @@ async def _login(self, binary: str) -> None: ``hermes hookdeck doctor`` reports it. """ if not self._config_path: - return + return True + binary = self.resolve_binary() if not self._api_key: logger.warning( "[hookdeck] No API key, so the CLI session cannot be pinned to " @@ -187,7 +187,7 @@ async def _login(self, binary: str) -> None: "the two agree." ) self._config_path = "" - return + return True try: process = await asyncio.create_subprocess_exec( binary, @@ -211,10 +211,14 @@ async def _login(self, binary: str) -> None: self._config_path, (stdout or b"").decode("utf-8", "replace").strip()[:300], ) + return False except asyncio.TimeoutError: - logger.warning("[hookdeck] `hookdeck ci` timed out after 30s") + logger.error("[hookdeck] `hookdeck ci` timed out after 30s") + return False except Exception as exc: # noqa: BLE001 # pragma: no cover - environment dependent - logger.warning("[hookdeck] `hookdeck ci` failed: %s", exc) + logger.error("[hookdeck] `hookdeck ci` failed: %s", exc) + return False + return True async def _supervise(self, binary: str) -> None: backoff = _BACKOFF_INITIAL diff --git a/tests/test_api.py b/tests/test_api.py index eab878d..05c1b04 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -7,8 +7,6 @@ from hookdeck.constants import ( API_KEY_ENV, CLI_API_KEY_ENV, - PROJECT_HEADER, - PROJECT_ID_ENV, ) @@ -81,32 +79,6 @@ async def test_the_namespaced_api_key_wins(monkeypatch): assert HookdeckAPI().api_key == "key_namespaced" -async def test_the_project_is_sent_as_a_header_when_pinned(): - """An org-level key can reach several projects; this says which.""" - seen: dict = {} - - def handler(request: httpx.Request) -> httpx.Response: - seen["headers"] = request.headers - return httpx.Response(200, json={}) - - api = HookdeckAPI("k", project_id="tm_abc", client=_client(handler)) - await api.list_events() - assert seen["headers"][PROJECT_HEADER] == "tm_abc" - - -async def test_no_project_header_when_nothing_is_pinned(monkeypatch): - monkeypatch.delenv(PROJECT_ID_ENV, raising=False) - seen: dict = {} - - def handler(request: httpx.Request) -> httpx.Response: - seen["headers"] = request.headers - return httpx.Response(200, json={}) - - api = HookdeckAPI("k", client=_client(handler)) - await api.list_events() - assert PROJECT_HEADER.lower() not in seen["headers"] - - async def test_non_2xx_carries_the_status_and_body(): def handler(request: httpx.Request) -> httpx.Response: return httpx.Response(422, text='{"data":["measures is required"]}') diff --git a/tests/test_cli.py b/tests/test_cli.py index 64b2934..ce80bac 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -157,3 +157,68 @@ def test_no_warning_in_sync_mode_where_the_cap_does_work(monkeypatch, capsys): url="https://example.com/hookdeck/r") ) assert "has no effect" not in capsys.readouterr().out + + +# ---------------------------------------------------------------------- +# doctor: the CLI and the API key must agree on a project +# ---------------------------------------------------------------------- + + +def _write_cli_config(path, body: str): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(body) + return path + + +def test_the_active_profile_decides_which_project_the_cli_uses(tmp_path): + """A CLI config is multi-section, with `profile` selecting the active one. + + Reading the first `project_id` in the file reports a mismatch that is not + real for anyone with more than one profile — worse than not checking. + """ + config = _write_cli_config( + tmp_path / "config.toml", + "profile = 'work'\n\n[default]\nproject_id = 'tm_personal'\n\n" + "[work]\nproject_id = 'tm_work'\n", + ) + assert cli._cli_config_project(config) == ("tm_work", True) + + +def test_the_default_profile_is_assumed_when_none_is_named(tmp_path): + config = _write_cli_config( + tmp_path / "config.toml", "[default]\nproject_id = 'tm_a'\n" + ) + assert cli._cli_config_project(config) == ("tm_a", True) + + +def test_a_missing_config_is_distinguished_from_one_without_a_project(tmp_path): + # doctor says something different about each: an absent file is fine + # because the gateway creates it, an unusable one is not. + assert cli._cli_config_project(tmp_path / "nope.toml") == ("", False) + present = _write_cli_config(tmp_path / "c.toml", "[default]\napi_key = 'k'\n") + assert cli._cli_config_project(present) == ("", True) + + +def test_a_project_mismatch_is_reported(tmp_path, monkeypatch): + config = _write_cli_config( + tmp_path / "config.toml", "[default]\nproject_id = 'tm_cli'\n" + ) + monkeypatch.setattr(cli, "_api_key_project", lambda: "tm_key") + check = cli._check_cli_project({"cli_config_path": str(config)}) + assert not check.ok + assert "tm_key" in check.message and "tm_cli" in check.message + + +def test_agreement_is_reported_as_fine(tmp_path, monkeypatch): + config = _write_cli_config( + tmp_path / "config.toml", "[default]\nproject_id = 'tm_same'\n" + ) + monkeypatch.setattr(cli, "_api_key_project", lambda: "tm_same") + assert cli._check_cli_project({"cli_config_path": str(config)}).ok + + +def test_a_config_the_gateway_has_not_created_yet_is_not_a_failure(tmp_path, monkeypatch): + monkeypatch.setattr(cli, "_api_key_project", lambda: "tm_key") + check = cli._check_cli_project({"cli_config_path": str(tmp_path / "absent.toml")}) + assert check.ok + assert "does not exist yet" in check.note diff --git a/tests/test_settings.py b/tests/test_settings.py index 08c2b58..fd7c2b8 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -5,11 +5,10 @@ from hookdeck.constants import ( MODE_ENV, PATH_ENV, - PROJECT_ID_ENV, SOURCE_ENV, WEBHOOK_SECRET_ENV, ) -from hookdeck.settings import AdapterSettings +from hookdeck.settings import AdapterSettings, default_cli_config_path MINIMAL = {"secret": "whsec_x", "routes": {"a": {"source": "s"}}} @@ -23,7 +22,7 @@ def test_defaults_are_the_conservative_ones(monkeypatch): # Off by default: cancelling retries discards traffic, and `hookdeck ci` # rewrites the operator's shared CLI config. assert settings.cancel_retries_on_unparseable is False - assert settings.cli_config_path.endswith("cli-config.toml") + assert settings.cli_config_path == str(default_cli_config_path()) def test_config_wins_over_the_environment(monkeypatch): @@ -114,8 +113,8 @@ def test_every_caller_resolves_the_same_ledger(tmp_path, monkeypatch): def test_the_default_is_used_when_nothing_is_configured(monkeypatch): - from hookdeck.settings import configured_state_path from hookdeck.ledger import default_state_path + from hookdeck.settings import configured_state_path monkeypatch.setattr("hookdeck.settings.load_hermes_config", dict) assert configured_state_path() == default_state_path() @@ -126,7 +125,30 @@ def test_config_still_outranks_both(monkeypatch): assert AdapterSettings.from_extra({"secret": "from_yaml"}).signing_secret == "from_yaml" -def test_the_project_can_be_pinned_for_an_org_level_key(monkeypatch): - monkeypatch.setenv(PROJECT_ID_ENV, "tm_from_env") - assert AdapterSettings.from_extra({}).project_id == "tm_from_env" - assert AdapterSettings.from_extra({"project_id": "tm_yaml"}).project_id == "tm_yaml" + + +def test_a_bare_cli_config_path_key_is_not_the_string_None(): + """`cli_config_path:` with no value parses as None in YAML. + + `str(None)` is "None", which would have the adapter pass + `--hookdeck-config None` and write a file by that name into its working + directory — while doctor inspected the default path instead. + """ + assert AdapterSettings.from_extra({"cli_config_path": None}).cli_config_path == str( + default_cli_config_path() + ) + + +def test_an_explicit_empty_cli_config_path_survives(): + # Distinct from absent: it means "use my own ambient `hookdeck login`". + assert AdapterSettings.from_extra({"cli_config_path": ""}).cli_config_path == "" + + +def test_a_tilde_in_cli_config_path_is_expanded(): + # The Hookdeck CLI does not expand `~` either, so an unexpanded path makes + # it create a directory literally named `~` in the working directory. + resolved = AdapterSettings.from_extra( + {"cli_config_path": "~/somewhere/cli.toml"} + ).cli_config_path + assert not resolved.startswith("~") + assert resolved.endswith("/somewhere/cli.toml") From 5780f2fd4bf28830f66a277545f3c4db4e1b3c92 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 11 Aug 2026 16:17:51 +0100 Subject: [PATCH 14/14] Bring the docs into line with the code, in main's structure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebasing onto main's README rewrite left the docs describing an older plugin. Correctness first: the quickstart exported `HOOKDECK_API_KEY` and `HOOKDECK_WEBHOOK_SECRET`, which the code no longer reads, so following the README would produce a gateway that refuses to start. `docs/security.md` named the old allowlist variable, and `examples/config.yaml` still offered `cli_login`, which no longer exists. `docs/operations.md` said the adapter deliberately does not authenticate the CLI. It now does, for reasons that section did not have when it was written, so it says the opposite and explains why. New material goes where main put reference material rather than back into the README. `docs/architecture.md` carries the topology and the ack-and-hand-back sequence diagram — both rendered before committing — plus the delivery pipeline and the three things called "CLI". `docs/limitations.md` gains what Hookdeck can do that this plugin does not ask it to, and the fact that CLI_DISCONNECTED events are recoverable with a reconnect-first ordering rule. The README keeps main's shape: it says which Hookdeck this is, since the platform is more than one product and Outpost points the other way. The "Verified end to end" section stays deleted, as main decided. The evidence lives in the pull request. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QiavrHa47c8mRR2kekHgVo --- README.md | 12 +++-- docs/architecture.md | 126 +++++++++++++++++++++++++++++++++++++++++++ docs/limitations.md | 60 +++++++++++++++++++-- docs/operations.md | 29 +++++++--- docs/security.md | 2 +- examples/config.yaml | 2 +- 6 files changed, 216 insertions(+), 15 deletions(-) create mode 100644 docs/architecture.md diff --git a/README.md b/README.md index bef322e..f0434d3 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,9 @@ **A durable, verified queue in front of your Hermes agent, so a webhook can trigger an agent run without the usual ways that goes wrong.** -Agent runs are not ordinary webhook handlers. They take seconds to minutes, cost money per execution, and must not run twice for the same event. Hermes's built-in webhook platform is fine for trying things out, but in production it drops bursts over 30/min and forgets duplicates after a restart. Any run that fails after the 202 is sent is simply lost. This plugin replaces that ingestion path with [Hookdeck](https://hookdeck.com), plus a local ledger that tracks the outcomes Hookdeck can't see. +Agent runs are not ordinary webhook handlers. They take seconds to minutes, cost money per execution, and must not run twice for the same event. Hermes's built-in webhook platform is fine for trying things out, but in production it drops bursts over 30/min and forgets duplicates after a restart. Any run that fails after the 202 is sent is simply lost. This plugin replaces that ingestion path with the [Hookdeck Event Gateway](https://hookdeck.com/docs), plus a local ledger that tracks the outcomes Hookdeck can't see. + +> Inbound only. This is the Event Gateway — third-party events arriving at your agent. It is not [Outpost](https://hookdeck.com/docs/outpost), which points the other way, and nothing here helps Hermes publish webhooks. ## Why @@ -31,11 +33,11 @@ pip install hermes-hookdeck && hermes plugins enable hookdeck git clone https://github.com/hookdeck/hermes-hookdeck ~/.hermes/plugins/hermes-hookdeck ``` -Configure two environment variables from your Hookdeck dashboard (Project Settings > Secrets): +Configure two environment variables from your Hookdeck dashboard (Project Settings > Secrets). They are prefixed `HOOKDECK_EG_` for the Event Gateway, since Hookdeck's platform is more than one product: ```bash -export HOOKDECK_API_KEY=... # provisions connections -export HOOKDECK_WEBHOOK_SECRET=... # verifies deliveries +export HOOKDECK_EG_API_KEY=... # provisions connections +export HOOKDECK_EG_WEBHOOK_SECRET=... # verifies deliveries ``` Then create a route and check the setup: @@ -95,6 +97,8 @@ A bundled `triage-webhook-failures` skill teaches the agent to group failures by ## Documentation +- [How it fits together](docs/architecture.md) — where each piece runs, the + delivery pipeline, and the three different things called "CLI" - [How the reliability works](docs/reliability.md) — verification, the run ledger and its idempotency rule, backpressure, ack modes, and why retry rather than replay diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 0000000..edfd8c8 --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,126 @@ +# How it fits together + +Where each piece runs, and what crosses your network boundary. + +```mermaid +flowchart LR + P["Provider
GitHub · Stripe · Shopify · …"] + + subgraph HD["Hookdeck Event Gateway — hosted"] + direction TB + SRC["Source
verifies the provider's
own signature"] + RULES["Connection rules
filter · deduplicate · retry"] + Q[("Event queue
holds what is not yet
delivered, within retention")] + SRC --> RULES --> Q + end + + subgraph GW["Your machine — one hermes gateway process"] + direction TB + AD["hookdeck adapter
verifies x-hookdeck-signature
deduplicates · admission control"] + LED[("Run ledger
SQLite, survives restarts")] + RUN["Agent run
prompt → tools → response"] + AD <--> LED + AD --> RUN + end + + P -->|"POST, signed by the provider"| SRC + Q -->|"cli mode
hookdeck listen holds an outbound
connection — no public URL"| AD + Q -->|"push mode
HTTPS to your reachable URL"| AD + + style HD fill:#f4f7ff,stroke:#4571d1,color:#26324d + style GW fill:#f3faf1,stroke:#3f8f3c,color:#1f3d1e +``` + +Two signatures, two different jobs. Hookdeck checks the *provider's* signature +at the edge — Stripe's, Shopify's, Twilio's, ~140 schemes — then signs its own +delivery. The adapter checks only that one, which is the whole point: Hermes +implements one verifier instead of one per provider. + +## The delivery that has to come back + +The diagram above is only the path in. What makes this more than a webhook +listener is the arrow it does not show — the adapter telling Hookdeck a run +failed, so the event returns instead of being forgotten: + +```mermaid +sequenceDiagram + autonumber + participant H as Hookdeck + participant A as Adapter + participant L as Ledger + participant R as Agent run + + H->>A: deliver — event id, attempt 1, x-hookdeck-signature + A->>A: verify · route · parse · filter + A->>L: is this new work? + L-->>A: yes — attempt 1 beats nothing seen + A-->>H: 202 accepted + Note over A,H: The ack goes out before the run finishes.
Recoverable in both directions, which is what lets
Hookdeck be the queue instead of the plugin owning one. + A->>R: dispatch + R-->>A: failed + A->>L: mark failed + A->>H: POST /events/{id}/retry + H->>A: deliver — same event id, attempt 2 + Note over L: attempt 2 > attempt 1, so this is a retry, not a duplicate.
A repeat of attempt 1 would be refused. +``` + +The attempt counter is what lets deduplication and retry coexist rather than +cancelling out. It is also how a gateway that dies at step 7 recovers: the +ledger row is still `running` at the next start, which by then can only be an +orphan, so the adapter asks for the same redelivery at step 9. See +[reliability](reliability.md) for the ack modes. + +## Two ways in + +Both modes run the same adapter and the same reliability machinery. They differ +only in how an event crosses your network boundary. + +| | `mode: cli` (default) | `mode: push` | +|---|---|---| +| Reachability | None needed — the connection is outbound | A public HTTPS URL | +| Suits | A laptop, a homelab box, anything behind NAT | A VPS, a container, anything with an address | +| Extra process | One `hookdeck listen` per route | None | +| Gateway-side throttling | Not available — CLI destinations have no rate limit | Delivery rate limits, delivery groups, issue triggers, alerting | +| Buffering while you are down | Only if you **pause** first | Yes; failed deliveries stay queued and retry | + +In `cli` mode the listener binds loopback only and is not reachable from the +network at all. In `push` mode it binds whatever `host` you configure, and the +signature check is the only thing in front of it. + +## Three things here are called "CLI" + +Worth separating once, because the quickstarts use all three: + +- **The Hookdeck CLI** (`hookdeck`) — a binary you install from Hookdeck, and + what makes `cli` mode work. You do not run it by hand: the adapter spawns + `hookdeck listen` itself, one process per route, and supervises it — + restarting with capped backoff if it dies, piping its output into the gateway + log. What you do need is version 2.3.2 or later. The adapter authenticates a + CLI session of its own; see [operations](operations.md). +- **`hermes hookdeck …`** — the operator commands this plugin adds: `setup`, + `status`, `pause`, `resume`, `retry`, `doctor`. These call the Hookdeck REST + API rather than the binary above, and work in both modes. +- **`hermes`** — Hermes' own CLI, which hosts all of the above. `hermes gateway` + runs the process the adapter lives in. + +## What the adapter does with one delivery + +The order is deliberate, and the security-relevant part is that nothing reads +the payload before the signature is checked: + +1. **Verify** `x-hookdeck-signature` against the raw bytes, in constant time. +2. **Route** — by the path segment Hookdeck was told to deliver to, else by + source name. No match is a 404, never a guess. +3. **Parse** as strict UTF-8 JSON or form encoding. Anything else is a 400 that + no retry can fix. +4. **Filter** — the route's `events` list, payload filters and route script. An + event the route does not want gets a 200, not an error. +5. **Deduplicate** against the ledger, *before* considering capacity, so a + repeat arriving at a busy moment is not deferred and then mistaken for new + work when it comes back. +6. **Admit or defer** — over `max_concurrent`, the answer is 503 with + `Retry-After` and nothing is written down. +7. **Dispatch**, then answer according to `ack_mode`, and record the run's real + outcome when it finishes. + +Each step either produces a response and stops, or hands the delivery on. diff --git a/docs/limitations.md b/docs/limitations.md index fc70d7d..08a9b6c 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -17,9 +17,18 @@ including those that only surface once you provision connections yourself. signing secret entered on the source in the Hookdeck dashboard. `setup` creates the source with the right verification shape but cannot invent the secret. -- The plugin does not poll. Hookdeck has no pull/consumer API — the Events API - is inspection-only, with no ack, lease or consumer group — so if you cannot - run the CLI and cannot expose a URL, it will not help you. +- Delivery is push-only, in both directions. Hookdeck pushes to the adapter and + the adapter pushes retry requests back; there is no lease-and-ack loop, and no + pull API to build one from — the Events API is for inspection, with no ack, + lease or consumer group. So if you can neither run the CLI nor expose a URL, + this plugin cannot help you. And "the event is safe in Hookdeck" holds because + a delivered-but-failed event stays retryable, not because anything holds a + lock on it — which is why boot recovery reconciles `running` ledger rows + itself. +- In `cli` mode the gateway needs an API key, not just a signing secret: it is + what pins the CLI session to the right project. Without one the adapter falls + back to your ambient `hookdeck login`, and the project it forwards from can + then differ from the one it manages. `doctor` reports that; it cannot fix it. - Delivery groups throttle per subject by rate, not by concurrency, because Hookdeck's group-level period is `second|minute|hour`. - Every recovery path is bounded by your plan's retention: 3 days on @@ -32,3 +41,48 @@ including those that only surface once you provision connections yourself. - Boot-time recovery re-runs an event whose run might in fact have completed in the instant before a crash. That is the at-least-once contract the whole design assumes; set `recover_on_boot: false` if it is wrong for your routes. + +## Hookdeck can do more than this plugin asks it to + +Everything above is what *cannot* be done. This is the other boundary: things +Hookdeck offers that the plugin does not wire up, so nobody mistakes the edge of +`hookdeck/api.py` for the edge of the product. Each is a candidate, not a +promise. + +**Getting events in.** The [Publish API](https://hookdeck.com/docs/api/publish.md) +sends a request to any source, authenticated with the same API key. Nothing here +calls it, and two uses stand out: a `hermes hookdeck test ` that puts a +real event through the real connection without waiting for a provider, and a way +for Hermes to enqueue durable work for itself. + +**Recovering events ignored while the CLI was disconnected.** The caveat above +says events arriving with no listener attached are discarded. That is what *this +plugin* does with them, not what Hookdeck can do: +[`POST /bulk/ignored-events/retry`](https://hookdeck.com/docs/api/bulk.md#bulk-retry-ignored-events) +takes a query filtered by `cause` and `webhook_id`, and `CLI_DISCONNECTED` is a +first-class cause. Retrying re-runs the *original request* through ingestion, so +the recovery is genuine rather than a status change. + +One ordering rule makes it work, and it is the whole trick: **reconnect first, +then retry.** The retry re-evaluates the same "no attached listen session" +condition that ignored the event, so retrying while still disconnected simply +produces another ignored event. + +**Bulk operations with the safety catch on.** `hookdeck_bulk_retry` is an +*agent-callable* tool that fires `POST /bulk/events/retry` immediately. Hookdeck +can estimate a bulk operation before running it (`GET /bulk/events/retry/plan`) +and cancel one in flight. An agent that could see "this would re-run 4,000 +events" before committing is a materially safer agent. + +**Requests, not just events.** A Hookdeck *request* is what the provider sent; +an *event* is one connection's copy. `/bulk/requests/retry` and +`/bulk/requests/replay` re-run the request, producing fresh events for every +matching connection — the right instrument after fixing a connection that was +misconfigured when the traffic arrived. + +**Alerting, shaping and metrics.** Issue triggers and notifications can report a +failing connection without anyone watching `hermes hookdeck status`; `setup` +provisions none. Transformations run JavaScript before delivery — the documented +workaround for the JSON-only limitation above is to add one by hand, and `setup` +could manage it. The dashboard reads `GET /metrics/queue-depth`; request, event +and attempt metrics would turn that number into a trend. diff --git a/docs/operations.md b/docs/operations.md index 5f67842..475447e 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -26,12 +26,29 @@ That is the durable path — paused events are held at `HOLD` and delivered on resume. Never reach for `disable` instead: it cancels pending events irrecoverably, as does deleting the connection. -The adapter does **not** run `hookdeck ci` to authenticate the CLI. That command -looks like a harmless idempotent login and is not: it rewrites the shared config -at `~/.config/hookdeck/config.toml`, swapping the stored key for a CLI session -key and switching the CLI's *active project*. Anyone using the CLI for other -work would find their environment repointed by starting a gateway. Log in -yourself with `hookdeck login`; set `cli_login: true` only if you accept that. +**The gateway keeps its own CLI session.** Two independent things decide "which +project": your API key decides what `setup`, `status` and the retry hand-back +act on, while the Hookdeck CLI's own config decides what `hookdeck listen` +forwards from. Nothing reconciles them, and when they differ every visible +signal says the gateway is fine — `setup` succeeds, the adapter logs that it is +listening, and only the tunnel's restart loop (`no connection found matching +filter`) says otherwise, while every event becomes a `CLI_DISCONNECTED` ignored +event. + +So the adapter authenticates a CLI config of its own from the API key it +already has, at `~/.hermes/hookdeck/cli-config.toml`, and passes +`--hookdeck-config` to every CLI call. Two projects cannot drift apart when +only one of them is configurable, and `hermes hookdeck doctor` compares them. + +This is deliberately not `hookdeck ci` against your shared config: that +rewrites `~/.config/hookdeck/config.toml` and switches the CLI's *active +project*, so anyone using the CLI for other work would find their environment +repointed by starting a gateway — and it does so even with `--local`, which +claims to write only to the current directory +([hookdeck-cli#332](https://github.com/hookdeck/hookdeck-cli/issues/332)). + +Set `cli_config_path: ""` to use your own `hookdeck login` session instead, and +accept that the two projects can then diverge. Use a CLI version of at least 2.3.2. Earlier ones stop delivering after a listen session expires without saying so, which from the gateway's side looks diff --git a/docs/security.md b/docs/security.md index 33d314b..0e3bcb2 100644 --- a/docs/security.md +++ b/docs/security.md @@ -22,5 +22,5 @@ exempts its own webhook platform from the user allowlist by enum member, reasoning that HMAC verification in the adapter *is* the authorization; the reasoning carries over but the membership test cannot, since this platform is `Platform.HOOKDECK`. The flag goes false whenever verification is off, so an -`INSECURE_NO_AUTH` route still falls under `HOOKDECK_ALLOWED_USERS` — narrower +`INSECURE_NO_AUTH` route still falls under `HOOKDECK_EG_ALLOWED_USERS` — narrower than core's exemption, which covers built-in webhook routes even unverified. diff --git a/examples/config.yaml b/examples/config.yaml index 3ecc05b..d275d9c 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -51,7 +51,7 @@ gateway: # Leave false. `hookdeck ci` rewrites ~/.config/hookdeck/config.toml # and switches the CLI's active project — not something starting a # gateway should do to a shared tool. Run `hookdeck login` instead. - # cli_login: false + # cli_config_path: "" # use your own `hookdeck login` session routes: # A GitHub PR reviewer. `source` is the Hookdeck source name.