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..0d8e7bc --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,99 @@ +# Tag-driven release to PyPI. +# +# 1. bump `__version__` in hookdeck/__init__.py (pyproject reads it from there) +# 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: +# 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: | + # 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 $prerelease 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/.gitignore b/.gitignore index b44940f..cefa201 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,8 @@ 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/ +.coverage +htmlcov/ 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 5a44c2a..d275d9c 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -17,7 +17,7 @@ gateway: # public_url: https://agent.example.com # Verified against x-hookdeck-signature. Prefer the env var. - # secret: ${HOOKDECK_WEBHOOK_SECRET} + # secret: ${HOOKDECK_EG_WEBHOOK_SECRET} # async_retry — ack 202 now, ask Hookdeck to redeliver if the run fails. # sync — hold the response until the run finishes (or times out). @@ -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. diff --git a/hookdeck/__init__.py b/hookdeck/__init__.py index ee85a80..a022b54 100644 --- a/hookdeck/__init__.py +++ b/hookdeck/__init__.py @@ -17,12 +17,17 @@ from pathlib import Path from typing import Any -from .constants import PLATFORM_NAME +from .constants import ( + ALLOW_ALL_USERS_ENV, + ALLOWED_USERS_ENV, + PLATFORM_NAME, + WEBHOOK_SECRET_ENV, +) logger = logging.getLogger(__name__) -__version__ = "0.1.0" -__all__ = ["register", "PLATFORM_NAME", "__version__"] +__version__ = "0.1.0rc1" +__all__ = ["PLATFORM_NAME", "__version__", "register"] PLATFORM_HINT = ( "You were triggered by a webhook delivered through Hookdeck, not by a " @@ -50,13 +55,13 @@ def _register_platform(ctx: Any) -> 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 e35ddb8..ef2c195 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 @@ -52,15 +52,20 @@ 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, header_name, ) -from .settings import AdapterSettings from .ledger import RunLedger +from .settings import AdapterSettings from .tunnel import HookdeckCLIMissing, HookdeckTunnel logger = logging.getLogger(__name__) @@ -135,8 +140,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 +152,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 @@ -184,7 +189,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 @@ -263,7 +268,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() @@ -285,16 +290,33 @@ 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, connection_name=route_name, binary=self.settings.cli_binary, - login=self.settings.cli_login, + 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: @@ -513,7 +535,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 +545,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,12 +574,17 @@ 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) - 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: @@ -625,7 +652,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 @@ -696,7 +723,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 @@ -715,7 +742,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 @@ -963,6 +990,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`.", @@ -986,7 +1014,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( @@ -1020,11 +1055,10 @@ 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) - self._acked_before_completion.discard(event_id) return True except HookdeckAPIError as exc: last = exc @@ -1054,7 +1088,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. @@ -1160,7 +1194,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 @@ -1210,7 +1244,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]: @@ -1258,7 +1292,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 @@ -1266,14 +1300,15 @@ def env_enablement() -> Optional[dict]: """ import os - if not os.getenv("HOOKDECK_WEBHOOK_SECRET"): + secret = os.getenv(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) if not value: diff --git a/hookdeck/api.py b/hookdeck/api.py index 26914c8..968cf2d 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, Mapping, Optional +from typing import TYPE_CHECKING, Any -from .constants import API_BASE_URL +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 import httpx @@ -81,13 +82,13 @@ 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.api_key = api_key or resolve_api_key() self.base_url = base_url.rstrip("/") self._timeout = timeout self._client = client @@ -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: @@ -123,7 +124,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: @@ -132,10 +133,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 @@ -155,6 +153,12 @@ async def request( except ValueError: return response.text + def _headers(self) -> dict[str, str]: + return { + "Authorization": f"Bearer {self.api_key}", + "Content-Type": "application/json", + } + # ------------------------------------------------------------------ # Connections # ------------------------------------------------------------------ @@ -167,6 +171,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") @@ -198,7 +205,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..a730827 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -15,18 +15,31 @@ 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 +from .constants import ( + API_KEY_ENV, + CLI_API_KEY_ENV, + DEFAULT_PATH, + DEFAULT_PORT, + MODE_ENV, + WEBHOOK_SECRET_ENV, + api_key, +) +from .ledger import RunLedger from .provision import ( build_connection_payload, routes_from_config, 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 @@ -44,7 +57,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 "" @@ -262,7 +275,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." @@ -333,7 +346,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) @@ -416,19 +429,23 @@ def render(self) -> None: def _check_credentials(extra: dict) -> list[Check]: + key = api_key() + secret = extra.get("secret") or os.getenv(WEBHOOK_SECRET_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.", ), ] @@ -442,6 +459,119 @@ def _check_routes(routes: dict) -> Check: ) +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 "", 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: + """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") + if configured == "": + path = Path.home() / ".config" / "hookdeck" / "config.toml" + source = f"your own session ({path})" + else: + # 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: + 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"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 " + f"from the other. The gateway will look healthy and every event will " + f"become a CLI_DISCONNECTED ignored event. {fix}", + ) + + 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" @@ -546,12 +676,13 @@ 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 os.getenv(MODE_ENV) or "cli" routes = routes_from_config(_load_hermes_config()) checks = [*_check_credentials(extra), _check_routes(routes)] if mode == "cli": checks += _check_cli(extra) + checks.append(_check_cli_project(extra)) else: checks.append( Check( @@ -572,7 +703,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 a827258..bf045de 100644 --- a/hookdeck/constants.py +++ b/hookdeck/constants.py @@ -8,15 +8,57 @@ from __future__ import annotations +import os + 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_" + +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" + + +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" 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/hookdeck/dashboard/plugin_api.py b/hookdeck/dashboard/plugin_api.py index f7986e5..2adeb63 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,10 +56,11 @@ 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.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() @@ -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/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/plugin.yaml b/hookdeck/plugin.yaml index 4304f07..efed4a8 100644 --- a/hookdeck/plugin.yaml +++ b/hookdeck/plugin.yaml @@ -18,37 +18,37 @@ 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_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/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..1fd565b 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, @@ -30,9 +31,14 @@ DEFAULT_RUN_TIMEOUT_SECONDS, DEFAULT_SYNC_TIMEOUT_SECONDS, INSECURE_NO_AUTH, + MODE_ENV, + PATH_ENV, + PORT_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") @@ -62,7 +68,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 {} @@ -82,10 +88,29 @@ 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" + + 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 +122,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 = "" @@ -122,14 +147,17 @@ 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 # ------------------------------------------------------------------ @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 @@ -139,10 +167,10 @@ def from_extra(cls, extra: Optional[Mapping[str, Any]]) -> "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 (os.getenv(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 {}), @@ -150,10 +178,10 @@ 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 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), 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)), @@ -186,10 +214,15 @@ def text(key: str, env: str = "", default: str = "") -> str: # An npm global shadowing a Homebrew install is the common case, # and PATH silently picks the older one. cli_binary=text("cli_binary", default="hookdeck"), - # 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:` 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), ) # ------------------------------------------------------------------ @@ -201,7 +234,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 @@ -236,7 +269,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/tools.py b/hookdeck/tools.py index 47099c0..3ab417f 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) @@ -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 @@ -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..cc10cb4 100644 --- a/hookdeck/tunnel.py +++ b/hookdeck/tunnel.py @@ -16,7 +16,10 @@ import logging import os import shutil -from typing import Optional +import socket + +from .constants import CLI_API_KEY_ENV +from .constants import api_key as resolve_api_key logger = logging.getLogger(__name__) @@ -30,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.""" @@ -53,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( @@ -65,11 +82,12 @@ 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._login_enabled = login + self._api_key = api_key or resolve_api_key() + #: A CLI config this gateway owns, kept away from the operator's own. + self._config_path = config_path 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 # ------------------------------------------------------------------ @@ -103,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 # ------------------------------------------------------------------ @@ -111,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)) @@ -126,53 +149,76 @@ async def stop(self) -> None: self._supervisor = None await self._terminate() - async def _login(self, binary: str) -> None: - """Non-interactive auth, off by default because it is destructive. + 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 + 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. - ``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 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. - 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. + 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: - return + if not self._config_path: + return True + binary = self.resolve_binary() 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." ) - 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." - ) + self._config_path = "" + return True 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], ) + return False except asyncio.TimeoutError: - logger.warning("[hookdeck] `hookdeck ci` timed out after 30s") - except Exception as exc: # pragma: no cover - environment dependent - logger.warning("[hookdeck] `hookdeck ci` failed: %s", exc) + logger.error("[hookdeck] `hookdeck ci` timed out after 30s") + return False + except Exception as exc: # noqa: BLE001 # pragma: no cover - environment dependent + logger.error("[hookdeck] `hookdeck ci` failed: %s", exc) + return False + return True async def _supervise(self, binary: str) -> None: backoff = _BACKOFF_INITIAL @@ -182,7 +228,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: @@ -211,7 +257,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/hookdeck/verify.py b/hookdeck/verify.py index b81c7d9..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 @@ -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/pyproject.toml b/pyproject.toml index 1fce3cf..414f7e7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,10 +1,44 @@ +# `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" -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" -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. @@ -12,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. @@ -22,12 +65,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 @@ -35,6 +123,9 @@ testpaths = ["tests"] 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()) 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_adapter.py b/tests/test_adapter.py index db804cf..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 @@ -123,6 +124,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"}} @@ -443,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": {}}} ) @@ -891,6 +910,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 # ---------------------------------------------------------------------- @@ -1032,6 +1083,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. diff --git a/tests/test_api.py b/tests/test_api.py index 59272be..05c1b04 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -4,6 +4,10 @@ import pytest from hookdeck.api import HookdeckAPI, HookdeckAPIError, _clean_params +from hookdeck.constants import ( + API_KEY_ENV, + CLI_API_KEY_ENV, +) def _client(handler) -> httpx.AsyncClient: @@ -50,12 +54,31 @@ 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_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 e0b72e6..ce80bac 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) @@ -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_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 969fb3c..fd7c2b8 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -2,13 +2,19 @@ import pytest -from hookdeck.settings import AdapterSettings +from hookdeck.constants import ( + MODE_ENV, + PATH_ENV, + SOURCE_ENV, + WEBHOOK_SECRET_ENV, +) +from hookdeck.settings import AdapterSettings, default_cli_config_path 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" @@ -16,13 +22,13 @@ 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 == str(default_cli_config_path()) 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" ) @@ -73,14 +79,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() @@ -107,8 +113,42 @@ 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() + + +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_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") 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-") 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")