Skip to content

Commit 83d6ecb

Browse files
garethxclaude
andcommitted
fix: round-2 review — two fix regressions, publish blockers, form bodies
Three of these were introduced by the previous round's fixes, which is the point of reviewing a diff rather than a codebase. A. The concurrency slot leaked when the runner threw. Wrapping dispatch in a try/catch made runner throws survivable, which exposed an asymmetry that had been unreachable: `activeRuns` was incremented before `runner.start()` and decremented at five separate later points, none of them a throw. After maxConcurrentRuns such throws the route stopped accepting anything, permanently, with nothing recorded. The slot now has one owner and is released in `finally` unless a background waiter takes it. B. Destroying the request stream on a body-read failure killed the socket the response shares, so a planned 413 became a connection reset — which Hookdeck retries, re-buffering the oversized body each time, while our own counter logged a cancellation that never reached the wire. Only the timeout case destroys now, which is the only one still reading. C. The config-error fallback route trusted the raw basePath. Since the invalid config may BE the basePath, `"/"` — rejected by the parser precisely because a prefix route there captures every Gateway request — would have been registered as a catch-all answering 503. Sanitised. D. A permanently un-retryable orphan came back on every boot: the row was never settled when retryEvent failed, so it was re-found forever, one duplicate dead-letter per boot, and consumed the recovery budget ahead of events that could actually be recovered. A 404 is retention and settles; anything else may be transient and is left for the next boot. Publishing: added LICENSE (MIT, as package.json and README already claimed), a CI workflow, homepage/bugs, and a `files` field so npm ships the plugin rather than the test suite. Genericised references to sibling plugins that may not be public, and removed an internal connection name from the smoke script — mutations there now require naming the connection explicitly. Also: form-encoded bodies are supported, since Twilio- and Slack-style providers could otherwise never pass an ingress whose premise is "any provider Hookdeck verifies"; a transport fault after the 202 re-queues or dead-letters instead of only warning; agent turns document plainly that they are fire-and-forget on the TaskFlow transport, with maxAgentRetries warned about alongside sync; the shutdown budget is wired and shutdown no longer pauses routes that no longer exist; a deliberate pause survives a tunnel reconnect, where a shutdown pause is still lifted; pending auto-resume timers are cancelled on service stop; disk-mode doctor no longer reports the live Gateway's work as interrupted; and bulk replay refuses in http mode rather than reporting success having matched nothing. One found by writing the tests: `stop()` registered its exit listener after sending SIGTERM, so a child that exits promptly was missed and teardown waited out the whole grace period before SIGKILLing a process that had already gone. 586 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 87a090e commit 83d6ecb

30 files changed

Lines changed: 1048 additions & 149 deletions

.env.example

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99
# a project you care about is still the wrong place to point it.
1010
HOOKDECK_TEST_API_KEY=
1111

12-
# Optional. Defaults to https://api.hookdeck.com/2025-07-01
12+
# Optional, and read from the ENVIRONMENT only — this file is parsed for
13+
# HOOKDECK_TEST_API_KEY and the AGENT_TEST_* keys, nothing else.
14+
# Defaults to https://api.hookdeck.com/2025-07-01
1315
# HOOKDECK_API_BASE=
1416

1517
# HOOKDECK_TEST_API_KEY above is also picked up by `npm run test:agent`. With it
@@ -26,3 +28,8 @@ HOOKDECK_TEST_API_KEY=
2628
# AGENT_TEST_ANTHROPIC_API_KEY=
2729
# AGENT_TEST_OPENAI_API_KEY=
2830
# AGENT_TEST_MODEL=anthropic/claude-haiku-4-5 # must be an id this openclaw build knows
31+
#
32+
# To let the smoke test CHANGE real Hookdeck state — it acknowledges one issue —
33+
# set both of these. Without them the run is read-only by construction.
34+
# AGENT_TEST_ALLOW_MUTATIONS=1
35+
# AGENT_TEST_MUTATION_CONNECTION=my-test-connection

.github/workflows/ci.yml

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
name: CI
2+
3+
on:
4+
push:
5+
branches: [main]
6+
pull_request:
7+
8+
jobs:
9+
test:
10+
runs-on: ubuntu-latest
11+
steps:
12+
- uses: actions/checkout@v4
13+
- uses: actions/setup-node@v4
14+
with:
15+
node-version: 22
16+
cache: npm
17+
- run: npm ci
18+
- run: npm run typecheck
19+
# Excludes test/live, which needs a Hookdeck project and an API key.
20+
- run: npm test

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,4 @@ dist/
77
!.env.example
88
coverage/
99
.vitest/
10+
*.tgz

LICENSE

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
MIT License
2+
3+
Copyright (c) 2026 Hookdeck
4+
5+
Permission is hereby granted, free of charge, to any person obtaining a copy
6+
of this software and associated documentation files (the "Software"), to deal
7+
in the Software without restriction, including without limitation the rights
8+
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
9+
copies of the Software, and to permit persons to whom the Software is
10+
furnished to do so, subject to the following conditions:
11+
12+
The above copyright notice and this permission notice shall be included in all
13+
copies or substantial portions of the Software.
14+
15+
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
16+
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
17+
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
18+
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
19+
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
20+
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
21+
SOFTWARE.

README.md

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,11 @@ You do not need to configure destination auth either. CLI destinations default t
9797
| `maxConcurrent` | `4` | Local admission control. In CLI transport this is the **only** limit — CLI destinations carry no `rate_limit` field. |
9898
| `busyRetryAfterSeconds` | `10` | `Retry-After` sent when deferring at capacity. |
9999
| `deferAttemptLimit` | `5` | Deferrals of the same event before the short `Retry-After` is dropped and exponential backoff takes over. Capacity that has not recovered after this many attempts is not the transient condition a short interval assumes. |
100-
| `dedupe.ttlHours` | `168` | Ledger retention. Must exceed Hookdeck's one-week retry ceiling. |
100+
| `pause.onShutdown` | `true` | Pause the connection before stopping the listener, so events are held rather than discarded. |
101+
| `pause.shutdownTimeoutMs` | `5000` | Budget for the whole teardown: pausing, draining and stopping children. |
102+
| `catchUp.enabled` | `true` | After a reconnect, replay requests that arrived while nothing was listening. |
103+
| `catchUp.minGapSeconds` | `30` | Below this, an outage is not worth a bulk replay. |
104+
| `dedupe.ttlHours` | `168` | Ledger retention, matching Hookdeck's one-week retry ceiling. Raise it if you extend retries beyond a week. |
101105
| `safety.allowRetryCancel` | `false` | See [Retry cancellation](#retry-cancellation). |
102106
| `routes.<id>.source` || **Required.** Hookdeck source name. |
103107
| `routes.<id>.path` | `/<id>` | Appended to `ingress.basePath`. Matched as a prefix — see below. |
@@ -261,7 +265,7 @@ Without `apiKey`, orphans are still detected, settled and dead-lettered — they
261265

262266
## Agent tools
263267

264-
Eight tools, matching the shared contract's five operator verbs plus two read tools an agent host benefits from more than a CLI does.
268+
Eight tools: the shared contract's five operator verbs `setup`, `status`, `pause`/`resume`, `replay`, `doctor` — plus three an agent host benefits from more than a CLI does. Two of those correlate what Hookdeck saw with what we did (`hookdeck_recent_deliveries`, `hookdeck_inspect_event`); the third, `hookdeck_issues`, is the dead-letter queue's own lifecycle.
265269

266270
| Tool | Answers |
267271
|---|---|
@@ -341,8 +345,9 @@ Signature headers and resolved secrets are redacted from logs.
341345

342346
Not yet implemented:
343347

344-
- **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.
348+
- **Agent turns are fire-and-forget.** They run through TaskFlow `run_task`, which exposes flow state rather than a completion signal, so the delivery is acknowledged as soon as the run *starts*. Concretely: `ackMode: "sync"` does not wait, `maxAgentRetries` never fires, and a crash mid-run is **not** re-queued by boot recovery — the ledger row is already `succeeded`. Run durability belongs to the flow record from that point, and Hookdeck's guarantee covers delivery rather than completion. `deliver` and `lane` are likewise recorded but not passed to the turn. Every one of these warns at startup rather than failing quietly. `taskflow` and `wake` dispatch are unaffected.
345349
- **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.
350+
- **Form-encoded and JSON bodies only.** `application/x-www-form-urlencoded` (Twilio, Slack) and `application/json` are parsed; anything else is rejected permanently.
346351
- **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.
347352

348353
## Development
@@ -353,11 +358,11 @@ npm test
353358
npm run typecheck
354359
```
355360

356-
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.
361+
586 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.
357362

358363
## Shared reliability contract
359364

360-
This plugin conforms to a contract shared with the Hookdeck plugins for Hermes Agent and n8n, so that "what happens when the run fails" has the same answer in all three: the same verification rule, the same attempt-count deduplication, the same admission-control semantics, and the same operator verbs.
365+
This plugin conforms to a contract shared across Hookdeck's agent-platform plugins, so that "what happens when the run fails" has the same answer in each: the same verification rule, the same attempt-count deduplication, the same admission-control semantics, and the same operator verbs.
361366

362367
Where this plugin adds something the contract does not require — retry cancellation, last-attempt dead-lettering — it defaults to off, so out-of-the-box wire behaviour matches its siblings.
363368

index.ts

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
} from "./src/ingress/handler.js";
3131
import { deferFor, retryable } from "./src/protocol/outcome.js";
3232
import { reconcileOrphans } from "./src/recovery.js";
33+
import { cancelAllAutoResumes } from "./src/tools/pause.js";
3334
import { registerHookdeckTools } from "./src/tools/index.js";
3435
import type { ToolDeps } from "./src/tools/deps.js";
3536
import { openDiskState } from "./src/tools/state.js";
@@ -59,6 +60,24 @@ import {
5960

6061
const PLUGIN_ID = "hookdeck";
6162

63+
const DEFAULT_BASE_PATH = "/hookdeck";
64+
65+
/**
66+
* Picks a safe ingress path for the config-error route.
67+
*
68+
* Never trusts the value far enough to register a route that would swallow
69+
* traffic belonging to something else.
70+
*/
71+
function fallbackBasePath(pluginConfig: unknown): string {
72+
const raw = (pluginConfig as { ingress?: { basePath?: unknown } })?.ingress
73+
?.basePath;
74+
if (typeof raw !== "string") return DEFAULT_BASE_PATH;
75+
76+
const trimmed = raw.trim().replace(/\/+$/, "");
77+
if (!trimmed.startsWith("/") || trimmed === "") return DEFAULT_BASE_PATH;
78+
return trimmed;
79+
}
80+
6281
interface Runtime {
6382
ledger: Ledger;
6483
deadLetter: DeadLetterLog;
@@ -96,18 +115,15 @@ export default definePluginEntry({
96115
`config error at ${problem.path}: ${problem.message}`,
97116
);
98117
}
99-
// The configured basePath, not the default: a deployment that moved the
100-
// ingress would otherwise answer 404 on its real path during a config
101-
// error, and the events would be lost rather than held.
102-
const basePath =
103-
typeof (api.pluginConfig as { ingress?: { basePath?: unknown } })
104-
?.ingress?.basePath === "string"
105-
? ((api.pluginConfig as { ingress: { basePath: string } }).ingress
106-
.basePath satisfies string)
107-
: "/hookdeck";
118+
// The configured basePath where it is usable, so a deployment that moved
119+
// its ingress still holds events rather than 404ing them. Sanitised
120+
// first: the config being invalid is the whole reason we are here, and
121+
// the invalid part may BE the basePath — `"/"` is rejected by the parser
122+
// precisely because a prefix route there captures every Gateway request.
123+
const basePath = fallbackBasePath(api.pluginConfig);
108124

109125
api.registerHttpRoute({
110-
path: basePath.startsWith("/") ? basePath : `/${basePath}`,
126+
path: basePath,
111127
auth: "plugin",
112128
match: "prefix",
113129
replaceExisting: true,
@@ -549,6 +565,11 @@ export default definePluginEntry({
549565
// services are stopped BEFORE those hooks run, and `gateway_stop` is
550566
// capped at 5s. Connection pause and CLI teardown arrive with the
551567
// managed transport.
568+
// Before anything is torn down: a pending auto-resume holds the deps
569+
// it was created with, and firing after a restart would write through
570+
// the previous run's stores.
571+
cancelAllAutoResumes();
572+
552573
const active = runtime;
553574
runtime = undefined;
554575
hostConfig = undefined;

openclaw.plugin.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -515,7 +515,7 @@
515515
"allowRetryCancel": {
516516
"type": "boolean",
517517
"default": false,
518-
"description": "Permit 'Retry-After: -1' to cancel Hookdeck's automatic retries on permanently-invalid input. Off by default: with it off, wire behaviour matches the sibling Hermes and n8n plugins exactly."
518+
"description": "Permit 'Retry-After: -1' to cancel Hookdeck's automatic retries on permanently-invalid input. Off by default: with it off, wire behaviour matches Hookdeck's other agent-platform plugins exactly."
519519
}
520520
}
521521
},

package.json

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,20 @@
4444
},
4545
"scripts": {
4646
"test": "vitest run --exclude \"test/live/**\"",
47-
"test:watch": "vitest",
47+
"test:watch": "vitest --exclude \"test/live/**\"",
4848
"typecheck": "tsc --noEmit",
4949
"test:live": "vitest run test/live",
5050
"test:agent": "bash scripts/agent-smoke.sh"
51-
}
51+
},
52+
"homepage": "https://github.com/hookdeck/hookdeck-openclaw#readme",
53+
"bugs": {
54+
"url": "https://github.com/hookdeck/hookdeck-openclaw/issues"
55+
},
56+
"files": [
57+
"index.ts",
58+
"src",
59+
"openclaw.plugin.json",
60+
"README.md",
61+
"LICENSE"
62+
]
5263
}

scripts/agent-smoke.sh

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ HOOKDECK_KEY="$(read_env HOOKDECK_TEST_API_KEY)"
4747
# Off unless explicitly asked for. With a live key this run can then change
4848
# real Hookdeck state, so it must be a deliberate act, never a default.
4949
ALLOW_MUTATIONS="${AGENT_TEST_ALLOW_MUTATIONS:-}"
50+
# Which connection's issue the mutation question targets. Scoped on purpose:
51+
# "acknowledge the oldest issue" would let the model pick anything in the
52+
# project.
53+
MUTATION_CONNECTION="${AGENT_TEST_MUTATION_CONNECTION:-}"
5054

5155
if [ -z "$ANTHROPIC_KEY" ] && [ -z "$OPENAI_KEY" ]; then
5256
echo "No model key found. Add AGENT_TEST_ANTHROPIC_API_KEY or AGENT_TEST_OPENAI_API_KEY to .env.local"
@@ -152,10 +156,17 @@ if [ -n "$HOOKDECK_KEY" ]; then
152156
# These only mean something with a key: without one the tools say they need
153157
# an operator, which is the other half of what this script proves.
154158
ask "Using the hookdeck tools, tell me about any open Hookdeck Issues. What kind are they, and what would I have to do to clear one?"
159+
if [ -n "$ALLOW_MUTATIONS" ] && [ -z "$MUTATION_CONNECTION" ]; then
160+
echo
161+
echo "AGENT_TEST_ALLOW_MUTATIONS is set but AGENT_TEST_MUTATION_CONNECTION is not."
162+
echo "Set it to the connection whose issue may be acknowledged, so the run cannot"
163+
echo "pick an arbitrary one from your project."
164+
exit 2
165+
fi
155166
if [ -n "$ALLOW_MUTATIONS" ]; then
156167
# Scoped to one connection on purpose. "Acknowledge the oldest issue" would
157168
# let the model pick anything in the project.
158-
ask "Acknowledge the oldest open Hookdeck issue for the hermes-livetest connection, then confirm what changed and what did NOT change."
169+
ask "Acknowledge the oldest open Hookdeck issue for the $MUTATION_CONNECTION connection, then confirm what changed and what did NOT change."
159170
else
160171
# The refusal is the point: the correct answer names tools.allowMutations.
161172
ask "Acknowledge the oldest open Hookdeck issue for me."
@@ -175,6 +186,13 @@ if [ -n "$HOOKDECK_KEY" ]; then
175186
echo "failing and how. And the last question asked for a MUTATION on purpose —"
176187
echo "the correct outcome is a refusal naming tools.allowMutations, not an"
177188
echo "acknowledged issue."
189+
if [ -n "$ALLOW_MUTATIONS" ] && [ -z "$MUTATION_CONNECTION" ]; then
190+
echo
191+
echo "AGENT_TEST_ALLOW_MUTATIONS is set but AGENT_TEST_MUTATION_CONNECTION is not."
192+
echo "Set it to the connection whose issue may be acknowledged, so the run cannot"
193+
echo "pick an arbitrary one from your project."
194+
exit 2
195+
fi
178196
if [ -n "$ALLOW_MUTATIONS" ]; then
179197
echo
180198
echo "MUTATIONS WERE ENABLED for this run, so the acknowledge really wrote."

0 commit comments

Comments
 (0)