fix: remaining review findings — boot ordering, recovery, limits, docs

Completes the review backlog.

Boot ordering (10). Under CLI transport, unpause and catch-up ran before
the listeners attached — and an event delivered with no session attached
is discarded rather than queued, so recovery replayed the outage window
into nothing. Recovery is now driven per route by the listener's
`onConnected`, which also means a tunnel that drops and returns recovers
its own window instead of waiting for the next boot. Clearing the
disconnect cursor on "replay accepted" is sound in that order: with a
session attached, a later failure is CLI_UNAVAILABLE and stays in the
retry pipeline. Modes with no attach event recover immediately, as
before. `start()` is now idempotent — a second call would have orphaned
children with no handle left to kill them.

Recovery (14) settled the ledger row before asking Hookdeck to redeliver;
a crash between the two left a terminal row, no orphan, no Issue and no
dead-letter record. The order is inverted, and the over-budget path
records its breadcrumb before settling.

The in-dispatch capacity guard (12) returned `deferred`, which promises
that a background run owns the row and will settle it. No run started, so
the row stayed `running` forever, invisible to in-process recovery.

Replay protection (15). The HMAC covers the body only: the event id and
attempt count are unsigned, the secret is project-level, and no timestamp
is signed. The comment claiming otherwise now says exactly what the
scheme proves, the README lists it under Limitations, and an implausible
attempt count is discarded rather than recorded — otherwise anyone able
to replay a captured body could retire every legitimate redelivery of an
event as a duplicate.

A configured-but-unresolvable API key (16) reported "no API key is
configured", sending someone to fix a config that was already correct. A
secretRef needs the host secret runtime, which only the Gateway's service
start receives, so tools now distinguish "unavailable here" from
"absent".

Rate limits (17) get their own code and carry Retry-After; a replay
batch stops on the first 429 and says how many of how many ran, rather
than reporting a generic error per remaining event.

Also: the raw-body timeout destroys the stream instead of buffering on;
the readiness-timeout kill escalates to SIGKILL like stop() does; backoff
jitter is clamped so maxDelayMs is really the maximum; prerelease
versions compare per semver, not lexically, so beta.10 is above beta.9;
the config-error route honours a custom basePath and still registers
hookdeck_status, since a config error is exactly when someone asks
whether webhooks are working; and `deliver`, `lane` and `sync` warn at
startup that the TaskFlow transport does not carry them.

raw-body.ts and node-spawn.ts had no tests; both now have suites, the
latter against a fake child_process covering the error-without-exit case.

559 tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: garethx

README.md

@@ -341,7 +341,9 @@ Signature headers and resolved secrets are redacted from logs.
 
 Not yet implemented:
 
-- **No completion tracking for agent turns.** See [Agent turns](#agent-turns) — `sync` and `maxAgentRetries` need a completion hook the TaskFlow transport does not provide.
+- **No completion tracking for agent turns.** See [Agent turns](#agent-turns). Agent turns run through TaskFlow `run_task`, which exposes flow state rather than a completion signal, so `ackMode: "sync"` behaves as `async_retry`, and `deliver` and `lane` are recorded but not passed to the turn. Each is warned about at startup rather than failing quietly.
+- **A signature authenticates the body, not the headers.** Hookdeck's HMAC covers the raw body only, with a project-level secret and no signed timestamp. So the event id and attempt count arrive unauthenticated, and a captured `(body, signature)` pair stays valid. Deduplication is what provides replay protection, an implausible attempt count is discarded rather than recorded, and provider verification at the Source is the layer that keeps unsigned traffic out in the first place.
+- **List endpoints read the first page only.** `hookdeck_issues` and `hookdeck_recent_deliveries` report a real total from the count endpoint, but return one page of results.
 
 ## Development
 
@@ -351,7 +353,7 @@ npm test
 npm run typecheck
 ```
 
-526 tests, no Gateway or Hookdeck account required. Signature vectors are computed independently with `openssl`, `test/http-integration.test.ts` exercises the pipeline over a real socket including multi-byte UTF-8 and multi-chunk bodies, the store suites inject write failures at an exact call to prove the degradation rule, and `test/store-io.test.ts` runs against a real filesystem because that is the only place durability actually lives.
+559 tests, no Gateway or Hookdeck account required. Signature vectors are computed independently with `openssl`, `test/http-integration.test.ts` exercises the pipeline over a real socket including multi-byte UTF-8 and multi-chunk bodies, the store suites inject write failures at an exact call to prove the degradation rule, and `test/store-io.test.ts` runs against a real filesystem because that is the only place durability actually lives.
 
 ## Shared reliability contract
 

index.ts

@@ -1,5 +1,6 @@
 import { randomUUID } from "node:crypto";
 import { join } from "node:path";
+import { jsonResult } from "openclaw/plugin-sdk/core";
 import { definePluginEntry } from "openclaw/plugin-sdk/plugin-entry";
 
 import { parseHookdeckConfig } from "./src/plugin/config-parse.js";
@@ -95,8 +96,18 @@ export default definePluginEntry({
           `config error at ${problem.path}: ${problem.message}`,
         );
       }
+      // The configured basePath, not the default: a deployment that moved the
+      // ingress would otherwise answer 404 on its real path during a config
+      // error, and the events would be lost rather than held.
+      const basePath =
+        typeof (api.pluginConfig as { ingress?: { basePath?: unknown } })
+          ?.ingress?.basePath === "string"
+          ? ((api.pluginConfig as { ingress: { basePath: string } }).ingress
+              .basePath satisfies string)
+          : "/hookdeck";
+
       api.registerHttpRoute({
-        path: "/hookdeck",
+        path: basePath.startsWith("/") ? basePath : `/${basePath}`,
         auth: "plugin",
         match: "prefix",
         replaceExisting: true,
@@ -114,6 +125,39 @@ export default definePluginEntry({
           return true;
         },
       });
+
+      // A config error is exactly when someone asks "are webhooks working?".
+      // Registering nothing leaves the agent with no way to answer, so the one
+      // tool that can explain the situation is registered even here.
+      try {
+        api.registerTool(
+          {
+            name: "hookdeck_status",
+            label: "Hookdeck Status",
+            description:
+              "Reports the Hookdeck plugin's state. Start here. The plugin is currently misconfigured, " +
+              "so this returns the configuration errors that need fixing.",
+            parameters: { type: "object", properties: {} },
+            execute: async () =>
+              jsonResult({
+                ok: false,
+                code: "config_error",
+                note:
+                  "The Hookdeck plugin could not start: its configuration is invalid. Ingress answers " +
+                  "503 with a Retry-After, so events are held in Hookdeck rather than lost until this " +
+                  "is fixed.",
+                problems: parsed.problems,
+              }),
+          } as never,
+          { name: "hookdeck_status" },
+        );
+      } catch (err) {
+        api.logger?.warn?.(
+          `could not register hookdeck_status during a config error: ${
+            err instanceof Error ? err.message : String(err)
+          }`,
+        );
+      }
       return;
     }
 
@@ -232,18 +276,28 @@ export default definePluginEntry({
           const disk = await openDiskState({
             ttlHours: config.dedupe.ttlHours,
           });
+          // The host's secret runtime needs the live OpenClawConfig, which
+          // only the service start receives — and that has not run in this
+          // process. So a secretRef resolves to nothing here even though the
+          // key is configured perfectly well, and the difference has to be
+          // reported as "unavailable", not "absent": one is a deployment fault
+          // to go and fix, the other is normal.
           const apiKey = await resolveSecret(
             config.apiKey,
             "apiKey",
             hostSecrets,
           ).catch(() => undefined);
+          const keyConfiguredButUnresolvable =
+            apiKey === undefined && config.apiKey !== undefined;
+
           return {
             config,
             source: "disk",
             ledger: disk.ledger,
             deadLetter: disk.deadLetter,
             cursors: disk.cursors,
             logger: log,
+            ...(keyConfiguredButUnresolvable ? { apiKeyUnresolved: true } : {}),
             ...(apiKey !== undefined
               ? { client: createHookdeckClient({ apiKey }) }
               : {}),

src/dispatch/agent.ts

@@ -196,8 +196,13 @@ export function createAgentDispatcher(
       }
 
       if (activeRuns >= options.maxConcurrentRuns) {
+        // `settle: "failed"`, not `"deferred"`. The handler has already written
+        // the ledger row by the time dispatch runs, and `deferred` means "a
+        // background run owns this row and will settle it" — but no run
+        // started, so the row would stay `running` forever, invisible to
+        // in-process recovery. The response is still a plain retryable defer.
         return {
-          settle: "deferred",
+          settle: "failed",
           plan: deferFor(
             503,
             "busy",

src/hookdeck/client.ts

@@ -26,7 +26,14 @@ export interface HookdeckClientOptions {
 
 export type ApiResult<T> =
   | { ok: true; data: T }
-  | { ok: false; status?: number; code: string; message: string };
+  | {
+      ok: false;
+      status?: number;
+      code: string;
+      message: string;
+      /** Present on `rate_limited` when the response said how long to wait. */
+      retryAfterSeconds?: number;
+    };
 
 export interface HookdeckConnection {
   id: string;
@@ -232,6 +239,29 @@ export function createHookdeckClient(
         } catch {
           // Non-JSON error body; the status line is enough.
         }
+
+        // Rate limiting gets its own code and says when to come back. A caller
+        // looping over events — `hookdeck_replay` does, up to 100 — would
+        // otherwise report a generic failure per event, which reads as "those
+        // events are broken" rather than "slow down".
+        if (response.status === 429) {
+          const retryAfter = Number.parseInt(
+            response.headers.get("retry-after") ?? "",
+            10,
+          );
+          return {
+            ok: false,
+            status: 429,
+            code: "rate_limited",
+            message: Number.isFinite(retryAfter)
+              ? `${message} (rate limited; retry after ${retryAfter}s)`
+              : `${message} (rate limited)`,
+            ...(Number.isFinite(retryAfter)
+              ? { retryAfterSeconds: retryAfter }
+              : {}),
+          };
+        }
+
         return {
           ok: false,
           status: response.status,

src/ingress/handler.ts

@@ -3,7 +3,7 @@ import type {
   HookdeckPluginConfig,
   RouteConfig,
 } from "../plugin/config-types.js";
-import { decideAdmission } from "../protocol/admission.js";
+import { decideAdmission, plausibleAttempt } from "../protocol/admission.js";
 import {
   parseHookdeckDelivery,
   type HookdeckDelivery,
@@ -392,7 +392,11 @@ async function runPipeline(
 
     // 13. Dispatch, bracketed by ledger writes. `begin` is awaited: it is the
     //     boundary before which we must not acknowledge anything.
-    await deps.ledger.begin(eventId, delivery.attemptCount ?? 1, { routeId });
+    await deps.ledger.begin(
+      eventId,
+      plausibleAttempt(delivery.attemptCount) ?? 1,
+      { routeId },
+    );
 
     let outcome: DispatchOutcome;
     try {

src/ingress/raw-body.ts

@@ -15,6 +15,21 @@ export interface RawBodySource extends AsyncIterable<
   headers?: Record<string, string | string[] | undefined>;
 }
 
+/**
+ * Stops a stream we have finished with.
+ *
+ * Optional because the interface is deliberately narrow — tests pass plain
+ * async iterables — but a real `IncomingMessage` always has it.
+ */
+function destroy(req: RawBodySource): void {
+  const destroyable = req as { destroy?: () => void };
+  try {
+    destroyable.destroy?.();
+  } catch {
+    // Already torn down.
+  }
+}
+
 export interface ReadRawBodyOptions {
   maxBytes: number;
   timeoutMs: number;
@@ -74,7 +89,14 @@ export async function readRawBody(
   })();
 
   try {
-    return await Promise.race([read, timeout]);
+    const result = await Promise.race([read, timeout]);
+
+    // Destroy on any failure, not just success-by-timeout. Without this the
+    // `for await` keeps buffering after we have answered, so a slow client can
+    // hold a request's worth of memory per connection for as long as it likes.
+    if (!result.ok) destroy(req);
+
+    return result;
   } finally {
     if (timer) clearTimeout(timer);
   }

src/plugin/config-parse.ts

@@ -277,12 +277,32 @@ export function parseHookdeckConfig(raw: unknown): ConfigParseResult {
       });
     }
 
-    if (route.dispatch.mode === "agent" && route.dispatch.deliver) {
-      warnings.push({
-        path: `routes.${routeId}.dispatch.deliver`,
-        message:
-          "deliver is enabled on a webhook-triggered route: an injected payload could cause an outbound message",
-      });
+    if (route.dispatch.mode === "agent") {
+      // Both are carried in the config and neither reaches the runner: agent
+      // turns go through TaskFlow `run_task`, which takes a goal and nothing
+      // else. Saying so is better than an operator setting `deliver: false` and
+      // believing it is doing something.
+      if (route.dispatch.deliver) {
+        warnings.push({
+          path: `routes.${routeId}.dispatch.deliver`,
+          message:
+            "deliver has no effect on the TaskFlow transport and is not passed to the agent turn; it is recorded for a future transport that can carry it",
+        });
+      }
+      if (route.dispatch.lane !== undefined) {
+        warnings.push({
+          path: `routes.${routeId}.dispatch.lane`,
+          message:
+            "lane has no effect on the TaskFlow transport and is not passed to the agent turn",
+        });
+      }
+      if (route.dispatch.ackMode === "sync") {
+        warnings.push({
+          path: `routes.${routeId}.dispatch.ackMode`,
+          message:
+            "sync needs a completion signal the TaskFlow transport does not provide; this route will behave as async_retry",
+        });
+      }
     }
 
     routes[routeId] = {

src/protocol/admission.ts

@@ -13,9 +13,23 @@
  *   When the attempt header is absent, admit only if the previous run for that
  *   event is recorded as failed.
  *
+ * Both inputs — the event id and the attempt count — arrive in unsigned
+ * headers, since the HMAC covers only the body. An absurd attempt count would
+ * otherwise retire every legitimate redelivery of that event as a duplicate,
+ * so one is treated as no attempt count at all.
+ *
  * Storage is the host's business; this rule is not.
  */
 
+/**
+ * Above this, an attempt count is not believable.
+ *
+ * Hookdeck's own ceiling is 50 automatic attempts, and manual retries and
+ * replays add few enough that a five-digit count means the header is wrong or
+ * hostile, not that an event has genuinely been tried that often.
+ */
+export const MAX_PLAUSIBLE_ATTEMPT = 10_000;
+
 export type LedgerStatus = "running" | "succeeded" | "failed" | "exhausted";
 
 export interface LedgerRow {
@@ -49,10 +63,27 @@ export type AdmissionDecision =
         "duplicate_attempt" | "in_flight" | "already_succeeded" | "exhausted";
     };
 
+/**
+ * Discards an attempt count we do not believe.
+ *
+ * Applied wherever the header is used, not only when admitting: recording an
+ * implausible value would raise the bar above every real redelivery of that
+ * event and retire them all as duplicates. The header is unsigned, so this is
+ * reachable by anyone who can replay a captured body.
+ */
+export function plausibleAttempt(
+  attemptCount: number | undefined,
+): number | undefined {
+  if (attemptCount === undefined) return undefined;
+  return attemptCount > MAX_PLAUSIBLE_ATTEMPT ? undefined : attemptCount;
+}
+
 export function decideAdmission(
   row: LedgerRow | undefined,
-  attemptCount: number | undefined,
+  rawAttemptCount: number | undefined,
 ): AdmissionDecision {
+  const attemptCount = plausibleAttempt(rawAttemptCount);
+
   if (row === undefined) return { admit: true, reason: "first_delivery" };
 
   if (attemptCount !== undefined) {

src/protocol/signature.ts

@@ -5,9 +5,21 @@ import { createHmac, timingSafeEqual } from "node:crypto";
  *
  *   base64( HMAC-SHA256( raw_body_bytes, signing_secret ) )
  *
- * The secret is project-level, not per-connection. No timestamp is signed, so
- * the signature carries no replay protection on its own — deduplication is what
- * provides that, and it is mandatory rather than optional.
+ * What this does and does not prove is worth being exact about, because the
+ * scheme is narrower than "the request is authentic":
+ *
+ *  - It proves the BODY came from a project holding the signing secret.
+ *  - It does not cover any header, so the event id, the attempt count and the
+ *    source name all arrive unauthenticated.
+ *  - The secret is project-level, so a signature does not bind a body to a
+ *    particular route or connection.
+ *  - No timestamp is signed, so a captured (body, signature) pair stays valid
+ *    indefinitely.
+ *
+ * Deduplication is therefore load-bearing rather than an optimisation, and it
+ * is deduplication over UNSIGNED inputs: anyone holding one captured pair can
+ * re-present it with a fresh event id. `MAX_PLAUSIBLE_ATTEMPT` in
+ * `admission.ts` limits the damage from the worst version of that.
  *
  * Everything here is pure and operates on bytes. The raw body must be the exact
  * octets Hookdeck sent: re-serialising parsed JSON will not reproduce them.

src/recovery.ts

@@ -76,11 +76,10 @@ export async function reconcileOrphans(
   const ordered = [...orphans].sort((a, b) => a.updatedAt - b.updatedAt);
 
   for (const [index, row] of ordered.entries()) {
-    // Settle the row first, whatever happens next. Leaving it `running` would
-    // make the next boot treat it as an orphan again, forever.
-    await ledger.settle(row.eventId, "failed");
-
     if (index >= maxEvents || client === undefined) {
+      // Recorded before the row is settled: a crash between the two would
+      // otherwise leave a terminal row, no orphan for the next boot, and
+      // nothing anywhere saying the event was dropped.
       summary.skipped += 1;
       await deadLetter.record({
         eventId: row.eventId,
@@ -94,11 +93,16 @@ export async function reconcileOrphans(
         lastAttempt: false,
         attemptCount: row.attempt,
       });
+      await ledger.settle(row.eventId, "failed");
       continue;
     }
 
     const result = await client.retryEvent(row.eventId);
     if (result.ok) {
+      // Settled only once Hookdeck has accepted the redelivery. The other order
+      // loses the event entirely if the process dies between the two: a
+      // terminal row, no orphan, no Issue and no dead-letter record.
+      await ledger.settle(row.eventId, "failed");
       summary.retried += 1;
       logger.debug(`re-queued interrupted event ${row.eventId}`);
       continue;