Repository navigation
Keep the GitHub, Gmail, Chat and Outlook hooks in the kit's hook registry - #1076
Conversation
…stry Each driver kept its own reg:/caps: records, stub disposal and startHook() -> deliver() call around the kit's delivery queue. They now use HookRegistry for all of it, under the same storage keys, and alias their delivery interfaces to HookDelivery, which drops the suppression on the deliver call. Each calls set() inside ctx.storage.transactionSync(), so a registration that can't be stored leaves the hook as it was. Alarms, provider lifecycles, ingest filters, controllers and GitHub's disconnect sweep are unchanged.
|
LGTM! |
Preview:
|
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Approved — No issues found. This is a behavior-preserving refactor: it keeps the same storage keys and delivery steps, and now stores each registration and its stubs in one transaction.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No authorization, credential or injection boundary changes. Stub disposal is still handled, now by the registry.
📖 AGENTS.md: ✅ LGTM
No conflicts with the repository guidelines.
📋 Code Quality: ✅ LGTM
All four ports keep the same reg: and caps: storage keys and the same order of delivery calls. set() runs inside transactionSync with no awaits inside it, and no unused imports are left behind.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The plan update matches the change.
Reviewed commit: 4fef8324
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
#1072 added
@gadgets/gatekeeper-kit/hook-registryfor the fan-out behind a push-driven hook driver, with no driver on it. This ports the four that are onmain(GitHub, Gmail, Chat, Outlook): each drops its hand-writtenreg:{key}/caps:{key}records, stub disposal, queue cancel andstartHook()→delivery.deliver()call, and usesHookRegistryfor all of it. 66 lines added, 194 removed.reg:/caps:keys and wraps the sameHookDeliveryQueue, so hooks enabled before the port keep deliveringctx.storage.transactionSync(), so a registration that can't be stored leaves the hook as it was (the follow-up promised on Add a kit hook registry for the drivers that queue provider pushes #1072's review). Two overlapping alarm runs join instead of delivering twiceXHookDeliveryinterface is now an alias ofHookDelivery<…>, which removes the@ts-expect-erroron every deliver call. The one on each controller'senable()is still needed and stays#wakeBy,#reschedule), provider lifecycle, ingest filters and controller, and GitHub's disconnect, which still sweeps its storage and disposes eachcaps:record itself. Onlyset()is in the transaction, not the driver's neighbouring writes (Gmail's cursor, Outlook's watch record)transactionSyncon the storage fake in Outlook's Node test: GitHub workerd hooks (73), Chat (26), Gmail (29), Outlook (71), and the GitHub hooks integration test against a worker rebuilt from the ported sourcepnpm lint/pnpm testwere not run locally. No driver test makes aset()fail, so the transaction is covered only by the kit's own workerd test. Outlook's driver has no workerd test, only the Node one over fakesworkshop-backendorworkshop-sharedchange. PagerDuty is not onmain, so its port falls to that branch