diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2ece0c6..1cb4c46 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,6 +24,20 @@ jobs: - run: pip install -e '.[dev]' - run: ruff check --output-format=github . + dashboard: + # The dashboard bundle ships in the wheel and its two action buttons + # mutate a live Hookdeck project, so it needs a check of its own — the + # Python suite cannot see it. No npm install: the bundle takes everything + # from the injected SDK, so a fake one under node:vm exercises the real + # file, and node's own test runner runs it. + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: "22" + - run: node --test "tests/dashboard/*.test.mjs" + test: runs-on: ubuntu-latest strategy: diff --git a/docs/development.md b/docs/development.md index 8f4cd39..630863d 100644 --- a/docs/development.md +++ b/docs/development.md @@ -14,6 +14,21 @@ The tests stub the Hermes internals the adapter imports (`tests/hermes_stub.py`) so the ingest path — verification, dedup, admission control, ack modes, outcome reporting — is exercised without a Hermes checkout. +The dashboard bundle is JavaScript, so pytest cannot see it. It has its own +suite, run by node's built-in test runner: + +```bash +node --test "tests/dashboard/*.test.mjs" +``` + +No `package.json`, no install, nothing to build. The bundle takes React, its +hooks and its fetch from the injected `window.__HERMES_PLUGIN_SDK__`, so +`tests/dashboard/harness.mjs` fakes that SDK and runs the shipped file under +`node:vm` — the real `dist/index.js`, byte for byte. `createElement` returns +plain objects instead of rendering, because the questions worth asking are +which endpoint a button posts to and whether it is disabled, and neither needs +a DOM. + ## Code layout | Module | What lives there | diff --git a/hookdeck/api.py b/hookdeck/api.py index 968cf2d..93a4ef0 100644 --- a/hookdeck/api.py +++ b/hookdeck/api.py @@ -229,6 +229,18 @@ async def queue_depth( async def list_issues(self, **params: Any) -> Any: return await self.request("GET", "/issues", params=params) + async def count_issues(self, **params: Any) -> int: + """How many issues match, in total rather than on a page. + + The list endpoints paginate and their ``count`` is the page's own size, + so counting from a list means counting to whatever limit was asked for. + Issues have a dedicated count endpoint; events do not. + """ + result = await self.request("GET", "/issues/count", params=params) + if isinstance(result, dict): + return int(result.get("count") or 0) + return 0 + def run_sync(coro: Any) -> Any: """Run *coro* from synchronous CLI code.""" diff --git a/hookdeck/tools.py b/hookdeck/tools.py index 3ab417f..d6e9618 100644 --- a/hookdeck/tools.py +++ b/hookdeck/tools.py @@ -22,6 +22,11 @@ TOOLSET = "hookdeck" +#: How many failed events `hookdeck_queue_status` counts before giving up and +#: reporting a floor. High enough that a real inbox is counted exactly, low +#: enough that a badly broken one does not stall the tool. +FAILED_SCAN_LIMIT = 100 + def _run(coro: Any) -> Any: """Run *coro* from a synchronous tool handler. @@ -140,13 +145,18 @@ def hookdeck_queue_status(_args: dict) -> str: async def _go() -> str: async with HookdeckAPI() as api: depth = await api.queue_depth() - failed = await api.list_events(status="FAILED", limit=1) - issues = await api.list_issues(status="OPENED", limit=1) + failed = await api.list_events(status="FAILED", limit=FAILED_SCAN_LIMIT) + open_issues = await api.count_issues(status="OPENED") + seen = len(_models(failed)) return json.dumps( { "queue_depth": depth, - "failed_events_page_count": len(_models(failed)), - "open_issues_page_count": len(_models(issues)), + "failed_events": seen, + # Events have no count endpoint, so this is what one page + # holds. Saying when it is capped stops the model reporting a + # ceiling as though it were a total. + "failed_events_is_at_least": seen >= FAILED_SCAN_LIMIT, + "open_issues": open_issues, } ) diff --git a/tests/dashboard/bundle.test.mjs b/tests/dashboard/bundle.test.mjs new file mode 100644 index 0000000..071425a --- /dev/null +++ b/tests/dashboard/bundle.test.mjs @@ -0,0 +1,271 @@ +/** + * Tests for the shipped dashboard bundle. + * + * Run with `node --test tests/dashboard/`. No dependencies: the bundle takes + * everything it uses from the injected SDK, so a fake one is enough — see + * harness.mjs for why that beats adding a JS toolchain to test one file. + * + * The emphasis is the two buttons that mutate a live Hookdeck project. A + * mislabelled one is invisible on screen and only shows up in traffic. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; + +import { mount, mountRaw, nodesOfType, allText, buttonsLabelled } from "./harness.mjs"; + +const OVERVIEW = "/api/plugins/hookdeck/overview"; + +/** An overview payload with the fields the page reads, overridable per test. */ +function overview({ hookdeck = {}, local = {} } = {}) { + return { + hookdeck: { + configured: true, + depth: { max_depth: 0 }, + failed: [], + issues: [], + connections: [], + other_connection_count: 0, + ...hookdeck, + }, + local: { exists: true, counts: {}, failures: [], stranded: [], ...local }, + }; +} + +async function page(payload, extra = {}) { + const app = mount({ responses: { [OVERVIEW]: payload, ...extra } }); + await app.settle(); + return app; +} + +// ---------------------------------------------------------------------- +// The pause/resume toggle +// ---------------------------------------------------------------------- +// +// The label and the endpoint are both derived from `c.paused`. Invert that +// expression and the UI still reads correctly while doing the opposite — a +// button labelled "Pause" that resumes production traffic. + +test("a paused connection offers Resume, and resuming is what it posts", async () => { + const app = await page( + overview({ hookdeck: { connections: [{ id: "web_1", name: "stripe", paused: true }] } }), + ); + + const [button] = buttonsLabelled(app.tree, "Resume"); + assert.ok(button, "a paused connection should offer Resume"); + assert.equal(buttonsLabelled(app.tree, "Pause").length, 0); + + button.props.onClick(); + assert.deepEqual(app.requests.at(-1), { + path: "/api/plugins/hookdeck/connections/web_1/resume", + method: "POST", + }); +}); + +test("an active connection offers Pause, and pausing is what it posts", async () => { + const app = await page( + overview({ hookdeck: { connections: [{ id: "web_1", name: "stripe", paused: false }] } }), + ); + + const [button] = buttonsLabelled(app.tree, "Pause"); + assert.ok(button, "an active connection should offer Pause"); + assert.equal(buttonsLabelled(app.tree, "Resume").length, 0); + + button.props.onClick(); + assert.deepEqual(app.requests.at(-1), { + path: "/api/plugins/hookdeck/connections/web_1/pause", + method: "POST", + }); +}); + +test("each connection acts on its own id", async () => { + // One shared handler closing over the wrong row would pause a connection + // the operator did not click. + const app = await page( + overview({ + hookdeck: { + connections: [ + { id: "web_1", name: "a", paused: false }, + { id: "web_2", name: "b", paused: false }, + ], + }, + }), + ); + + const buttons = buttonsLabelled(app.tree, "Pause"); + assert.equal(buttons.length, 2); + buttons[1].props.onClick(); + assert.match(app.requests.at(-1).path, /web_2\/pause$/); +}); + +// ---------------------------------------------------------------------- +// Retry is a redelivery +// ---------------------------------------------------------------------- + +test("retrying a failed delivery posts to that event's retry endpoint", async () => { + const app = await page( + overview({ hookdeck: { failed: [{ id: "evt_1", response_status: 500, attempts: 2 }] } }), + ); + + const [button] = buttonsLabelled(app.tree, "Retry"); + button.props.onClick(); + assert.deepEqual(app.requests.at(-1), { + path: "/api/plugins/hookdeck/events/evt_1/retry", + method: "POST", + }); +}); + +test("retrying a failed agent run posts for that run's event", async () => { + const app = await page( + overview({ + local: { failures: [{ event_id: "evt_9", route: "stripe", status: "failed", agent_attempts: 2 }] }, + }), + ); + + const [button] = buttonsLabelled(app.tree, "Retry"); + button.props.onClick(); + assert.match(app.requests.at(-1).path, /events\/evt_9\/retry$/); +}); + +// ---------------------------------------------------------------------- +// `busy` — the guard against a double redelivery +// ---------------------------------------------------------------------- + +test("every action is disabled while a request is in flight", async () => { + // This plugin exists so one event runs the agent once. A double-click that + // fires two retries is that guarantee failing in the UI instead of the + // adapter. + const app = await page( + overview({ + hookdeck: { + failed: [{ id: "evt_1", response_status: 500, attempts: 1 }], + connections: [{ id: "web_1", name: "a", paused: false }], + }, + local: { failures: [{ event_id: "evt_9", route: "r", status: "failed", agent_attempts: 1 }] }, + }), + // Never resolves, so `busy` stays true and the tree can be inspected + // mid-flight. + { "/api/plugins/hookdeck/events/evt_1/retry": () => new Promise(() => {}) }, + ); + + buttonsLabelled(app.tree, "Retry")[0].props.onClick(); + const actionable = nodesOfType(app.tree, "Button").filter( + (b) => b.props.onClick && "disabled" in b.props, + ); + assert.ok(actionable.length >= 3, "retry, retry and pause all carry a disabled flag"); + for (const button of actionable) { + assert.equal(button.props.disabled, true); + } +}); + +test("a failed action leaves a way back rather than a dead panel", async () => { + // A failed action swaps the whole page for the error card, so the recovery + // that matters is that card's Retry — it reloads the overview and restores + // the page. (`busy` is not the guard here: the error card's button has no + // `disabled` binding, so it stays clickable regardless.) + const overviewPayload = overview({ + hookdeck: { connections: [{ id: "web_1", name: "a", paused: false }] }, + }); + const app = await page(overviewPayload, { + "/api/plugins/hookdeck/connections/web_1/pause": () => Promise.reject(new Error("nope")), + }); + + buttonsLabelled(app.tree, "Pause")[0].props.onClick(); + for (let i = 0; i < 20; i++) await Promise.resolve(); + assert.match(allText(app.tree), /nope/, "the failure is shown, not swallowed"); + + const [back] = buttonsLabelled(app.tree, "Retry"); + assert.ok(back, "a failed action must leave something to click"); + assert.equal(back.props.disabled, undefined, "the way back is never disabled"); + + back.props.onClick(); + await app.settle(); + assert.match(allText(app.tree), /Connections/, "the page comes back"); +}); + +// ---------------------------------------------------------------------- +// Loading against a host that does not have what it needs +// ---------------------------------------------------------------------- + +test("an SDK older than the bundle is a quiet no-op, not a throw", async () => { + // Upgrading the plugin before Hermes is the normal order, so the bundle + // will meet hosts that lack what it uses. Throwing here goes into the + // host's bundle loader and takes the whole dashboard down with it. + const hosts = [ + ["no SDK at all", {}], + ["an SDK but no plugin registry", { __HERMES_PLUGIN_SDK__: { React: {} } }], + ["a registry with no register()", { __HERMES_PLUGIN_SDK__: { React: {} }, __HERMES_PLUGINS__: {} }], + ]; + for (const [what, window] of hosts) { + let registered; + assert.doesNotThrow(() => { + registered = mountRaw(window); + }, `should load quietly against ${what}`); + assert.equal(registered.size, 0, `should register nothing against ${what}`); + } +}); + +test("the tab registers under the name its manifest declares", async () => { + const app = mount({ responses: { [OVERVIEW]: overview() } }); + assert.ok(app.registered.has("hookdeck"), "the host pairs the component to the tab by this name"); +}); + +// ---------------------------------------------------------------------- +// States an operator will actually see +// ---------------------------------------------------------------------- + +test("a missing API key explains itself and asks for nothing else", async () => { + const app = await page({ hookdeck: { configured: false }, local: {} }); + const text = allText(app.tree); + assert.match(text, /HOOKDECK_EG_API_KEY/); + assert.equal(nodesOfType(app.tree, "Button").length, 0, "nothing to click without a key"); +}); + +test("an empty queue says so rather than rendering a bare panel", async () => { + const app = await page(overview()); + assert.match(allText(app.tree), /None\./); + assert.match(allText(app.tree), /No connection matches a configured route yet\./); +}); + +test("hidden connections are counted without justifying themselves", async () => { + const app = await page(overview({ hookdeck: { other_connection_count: 26 } })); + const text = allText(app.tree); + assert.match(text, /26 other connection\(s\)/); + assert.match(text, /Only connections matching a configured route appear here\./); +}); + +test("stranded runs are surfaced with what to do about them", async () => { + const app = await page(overview({ local: { stranded: [{ event_id: "evt_1" }] } })); + assert.match(allText(app.tree), /still marked running after an hour/); + assert.match(allText(app.tree), /Restarting the gateway retries them/); +}); + +// ---------------------------------------------------------------------- +// Error rendering +// ---------------------------------------------------------------------- + +test("a rejection is shown whether or not it is an Error", async () => { + // `String((e && e.message) || e)` is written for both shapes; a plain + // string rejection is the one less likely to have been tried. + for (const [thrown, expected] of [ + [new Error("boom"), /boom/], + ["plain string failure", /plain string failure/], + ]) { + const app = mount({ + responses: { [OVERVIEW]: () => Promise.reject(thrown) }, + }); + await app.settle(); + assert.match(allText(app.tree), expected); + // And a way back, rather than a dead panel. + assert.ok(buttonsLabelled(app.tree, "Retry").length >= 1); + } +}); + +test("the error panel's Retry reloads the overview", async () => { + const app = mount({ responses: { [OVERVIEW]: () => Promise.reject(new Error("down")) } }); + await app.settle(); + const before = app.requests.length; + buttonsLabelled(app.tree, "Retry")[0].props.onClick(); + assert.equal(app.requests.at(-1).path, OVERVIEW); + assert.ok(app.requests.length > before); +}); diff --git a/tests/dashboard/harness.mjs b/tests/dashboard/harness.mjs new file mode 100644 index 0000000..7588ff3 --- /dev/null +++ b/tests/dashboard/harness.mjs @@ -0,0 +1,206 @@ +/** + * A fake Hermes plugin SDK, so the dashboard bundle can be tested as shipped. + * + * `dist/index.js` is committed rather than generated — no package.json, no + * bundler, no build step — and this keeps that property. It takes React, its + * hooks, its components and its fetch from `window.__HERMES_PLUGIN_SDK__`, so + * everything it touches is injectable: run it under `node:vm` with the SDK + * faked and you exercise the real file, byte for byte, with no dependencies. + * + * `createElement` returns plain objects rather than rendering, because every + * question worth asking here is about the tree and the requests — which + * endpoint a button posts to, whether it is disabled — and none of them need a + * DOM to answer. + */ + +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; +import vm from "node:vm"; + +const HERE = dirname(fileURLToPath(import.meta.url)); +export const BUNDLE = join(HERE, "..", "..", "hookdeck", "dashboard", "dist", "index.js"); + +/** A rendered node: `{type, props, children}` with `type` a string. */ +function createElement(type, props, ...children) { + const flat = []; + for (const child of children.flat(Infinity)) { + if (child !== null && child !== undefined && child !== false) flat.push(child); + } + return { + type: typeof type === "function" ? type.name || "fn" : String(type), + props: props || {}, + children: flat, + }; +} + +/** + * Hooks for a single component rendered repeatedly. + * + * State lives in a slot array keyed by call order — the same contract React + * relies on — so a setter can trigger a re-render and the next pass reads the + * updated value from the same slot. + */ +function makeHooks(rerender) { + let slots = []; + let cursor = 0; + const effects = []; + + return { + beginRender() { + cursor = 0; + effects.length = 0; + }, + runEffects() { + // Copied first: an effect may setState, which re-renders and refills + // the list while we are walking it. + for (const effect of [...effects]) effect(); + }, + hooks: { + useState(initial) { + const slot = cursor++; + if (slots.length <= slot) slots[slot] = initial; + const set = (next) => { + slots[slot] = typeof next === "function" ? next(slots[slot]) : next; + rerender(); + }; + return [slots[slot], set]; + }, + // Dependencies are ignored deliberately: this harness re-renders from + // scratch, so a stale-closure bug is not something it can observe, and + // pretending otherwise would be a test that lies about its own reach. + useCallback: (fn) => fn, + useEffect(fn) { + const slot = cursor++; + if (slots[slot] === undefined) { + slots[slot] = "ran"; + effects.push(fn); + } + }, + }, + reset() { + slots = []; + cursor = 0; + }, + }; +} + +/** + * Load the bundle with a faked SDK and render its page component. + * + * `responses` maps a request path to either a value to resolve or an `Error` + * to reject with — `{"/overview": {...}}`. + */ +export function mount({ responses = {}, sdk = {}, plugins = {} } = {}) { + const requests = []; + const registered = new Map(); + + const fetchJSON = (path, options) => { + requests.push({ path, method: (options && options.method) || "GET" }); + const answer = responses[path]; + if (answer instanceof Error) return Promise.reject(answer); + if (typeof answer === "function") return answer(); + return Promise.resolve(answer === undefined ? {} : answer); + }; + + let tree = null; + let component = null; + const hookStore = makeHooks(() => render()); + + function render() { + if (!component) return null; + hookStore.beginRender(); + tree = component({}); + return tree; + } + + const window = { + __HERMES_PLUGIN_SDK__: { + React: { createElement }, + // Plain strings: `createElement` stringifies a non-function type, so + // the node's `type` comes out as "Button" and stays greppable. + components: Object.fromEntries( + ["Card", "CardHeader", "CardTitle", "CardContent", "Badge", "Button"].map( + (name) => [name, name], + ), + ), + hooks: hookStore.hooks, + fetchJSON, + ...sdk, + }, + __HERMES_PLUGINS__: { + register: (name, fn) => registered.set(name, fn), + ...plugins, + }, + }; + + vm.runInNewContext(readFileSync(BUNDLE, "utf8"), { window, console }); + + component = registered.get("hookdeck") || null; + render(); + + return { + requests, + registered, + get tree() { + return tree; + }, + /** Render, then flush effects and any renders they cause. */ + async settle() { + hookStore.runEffects(); + for (let i = 0; i < 20; i++) await Promise.resolve(); + render(); + hookStore.runEffects(); + for (let i = 0; i < 20; i++) await Promise.resolve(); + return render(); + }, + }; +} + +/** + * Run the bundle against a window supplied verbatim. + * + * `mount` always builds a complete SDK, so it cannot express "this host does + * not have one" — which is exactly the case the bundle's opening guard exists + * for. Returns what the bundle registered, or throws if it threw. + */ +export function mountRaw(window) { + const registered = new Map(); + if (window && window.__HERMES_PLUGINS__ && !window.__HERMES_PLUGINS__.register) { + // left absent on purpose by the caller + } else if (window && window.__HERMES_PLUGINS__) { + window.__HERMES_PLUGINS__.register = (name, fn) => registered.set(name, fn); + } + vm.runInNewContext(readFileSync(BUNDLE, "utf8"), { window, console }); + return registered; +} + +/** Every node in the tree, depth first. */ +export function walk(node, out = []) { + if (!node || typeof node !== "object") return out; + out.push(node); + for (const child of node.children || []) walk(child, out); + return out; +} + +/** Nodes whose type matches, e.g. `nodesOfType(tree, "Button")`. */ +export function nodesOfType(tree, type) { + return walk(tree).filter((n) => n.type === type); +} + +/** Visible text of a node and everything under it. */ +export function textOf(node) { + return walk(node) + .flatMap((n) => n.children.filter((c) => typeof c === "string")) + .join(" "); +} + +/** All text in the tree, for "does it mention X" assertions. */ +export function allText(tree) { + return textOf(tree); +} + +/** Buttons whose own label matches `label` exactly. */ +export function buttonsLabelled(tree, label) { + return nodesOfType(tree, "Button").filter((b) => textOf(b).trim() === label); +} diff --git a/tests/test_tools.py b/tests/test_tools.py index a484312..4f5f1a9 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -58,6 +58,9 @@ async def list_events(self, **kw): async def list_issues(self, **kw): return await self._record("list_issues", **kw) + async def count_issues(self, **kw): + return await self._record("count_issues", **kw) or 0 + async def get_event_raw_body(self, event_id): return await self._record("get_event_raw_body", event_id) @@ -466,3 +469,49 @@ def test_every_required_parameter_is_a_declared_parameter(): params = schema["function"]["parameters"] missing = set(params["required"]) - set(params["properties"]) assert not missing, f"{name} requires undeclared {missing}" + + +# ---------------------------------------------------------------------- +# Queue status reports numbers, not page sizes +# ---------------------------------------------------------------------- + + +def test_queue_status_reports_the_real_number_of_open_issues(api): + # Counted from the dedicated endpoint. Counting from a listing would count + # to whatever limit was asked for — a project with four open issues used + # to be reported as one, and the model then guessed at what "1" meant. + api.responses["count_issues"] = 4 + api.responses["list_events"] = {"models": []} + status = json.loads(call("hookdeck_queue_status")) + + assert status["open_issues"] == 4 + assert calls_named(api, "count_issues") == [ + ("count_issues", (), {"status": "OPENED"}) + ] + # The old page-size fields are gone, not merely renamed alongside. + assert "open_issues_page_count" not in status + assert "failed_events_page_count" not in status + + +def test_queue_status_counts_failed_events_exactly_when_it_can(api): + api.responses["list_events"] = {"models": [{"id": f"evt_{i}"} for i in range(7)]} + status = json.loads(call("hookdeck_queue_status")) + assert status["failed_events"] == 7 + assert status["failed_events_is_at_least"] is False + + +def test_a_full_page_of_failures_is_flagged_as_a_floor(api): + # Events have no count endpoint, so a full page means "at least this + # many". Reporting it bare would let the model state a ceiling as a total. + api.responses["list_events"] = { + "models": [{"id": f"evt_{i}"} for i in range(tools.FAILED_SCAN_LIMIT)] + } + status = json.loads(call("hookdeck_queue_status")) + assert status["failed_events"] == tools.FAILED_SCAN_LIMIT + assert status["failed_events_is_at_least"] is True + + +def test_queue_status_asks_for_more_than_one_failure(api): + api.responses["list_events"] = {"models": []} + call("hookdeck_queue_status") + assert calls_named(api, "list_events")[0][2]["limit"] == tools.FAILED_SCAN_LIMIT