Conversation
github-actions
Bot
added
kernel
|
All contributors have signed the CLA ✍️ ✅ |
551f84c to
c678ffb
Compare
|
I have read the CLA Document and I hereby sign the CLA |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 2 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| async disable(hookId: string): Promise<void> { | ||
| this.ctx.storage.kv.delete(`hook:${hookId}`); | ||
| this.ctx.storage.sql.exec("DELETE FROM notification_receipts WHERE hook_id = ?", hookId); |
There was a problem hiding this comment.
🟡 Disabled subscriptions leak server capabilities
Disabling a subscription deletes hook:${hookId} without disposing its stored initiator. Replacement in enable and account cleanup discard the same capability likewise. Repeated subscription changes retain server resources indefinitely.
Prompt for agents
Audit StoredHook capability ownership in packages/gatekeeper-cloudflare/src/notifications.ts. The initiator stored under hook:* must be disposed whenever its record is replaced, disabled, or removed during revokeWithToken. Preserve the current hook and receipt semantics while following the disposal pattern used by the scheduler's StoredCapabilities lifecycle.
Was this helpful? React with 👍 or 👎 to provide feedback.
# Conflicts: # packages/workshop-backend/src/auth/login-flow.ts
| #hooks() { | ||
| return [...this.ctx.storage.kv.list<StoredHook>({ prefix: "hook:" })].map(([, hook]) => hook); | ||
| } |
There was a problem hiding this comment.
🔴 Every webhook leaks subscriber capabilities
Each #hooks() call deserializes every stored initiator without disposing it. Status checks, enablement, and every webhook retain server-side RPC resources indefinitely.
Learn more
Durable Object storage returns live RPC capabilities when a StoredHook is read. Each returned capability has caller-owned lifetime, even though another copy remains stored for later delivery. This helper materializes every initiator for status, quota, filtering, and delivery calls, but none of those paths releases the materialized stubs. Replacement, disable, and revocation also delete or overwrite stored capabilities without first loading and disposing them. The scheduler demonstrates the required ownership pattern in disposeCapabilities.
Example: With 100 subscribers, each incoming notification loads 100 initiator stubs. After 1,000 notifications, the receiver has created 100,000 unreleased server-side capabilities, even when every callback succeeds.
Recommended fix: Avoid deserializing hook values when only keys or counts are needed. For delivery, hold one snapshot, await all handoffs, then dispose every snapshot initiator in finally. On replacement, disable, and revocation, retrieve and dispose the removed initiator after the storage mutation, following the scheduler's capability ownership pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Sign-in grants billing access only. Resource grants own persistent bindings and subscriptions; | ||
| // preserve them even if expired. Their existing reconnect flow refreshes credentials in place. | ||
| if (uniqueName) { | ||
| let existing = this.#findConnectedAccountByIdentity(vendorId, uniqueName); | ||
| if (existing) { | ||
| if (existing.description.grantedResourceUrlPatterns?.length) { | ||
| await account.revoke(); | ||
| return existing.id; | ||
| } |
There was a problem hiding this comment.
🔍 Kernel prerequisite remains bundled
The PR changes repeated-login account preservation alongside notifications. Repository guidance requires kernel concerns to be split, and the description assigns this change to a separate PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /** Parse the generic ANS envelope. No fixed alert-type enumeration or event_id assumption. */ | ||
| export async function parseNotificationWebhook( |
There was a problem hiding this comment.
🔍 Production alert delivery remains unverified
Tests use synthetic notification envelopes. No policy-triggered incident has validated the production payload or delivery behavior, leaving the core integration assumption unresolved.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Superseded by a smaller composable design: a generic webhook gatekeeper owns ingress and subscriptions, while the Cloudflare gatekeeper will only provision the Notifications destination. |
1 participant
This MR adds notification webhooks and subscriptions to the existing Cloudflare gatekeeper, so Gadgets and agents can react to Cloudflare alerts without polling.
Screenshots
Connect your account. The setup uses the existing Cloudflare connector, with a short connection status and extra guidance under Details.
Receive alerts from Cloudflare. Enabling the first subscription creates a webhook destination in Cloudflare, which you can select on your notification policies.
Choose what each subscriber receives. Hooks use the standard Connections controls. This example has two independent subscriptions: one for all alert types and one filtered to a selected type.
See delivery working. This earlier test inbox shows synthetic alerts reaching Gadget callbacks, and
lastTestAtrecords a real Cloudflare ANS destination test. Its queue counters are from the previous implementation and have since been removed: the webhook now waits for subscriber handoff and returns 500 on failure so ANS can retry. Successful handoffs are deduplicated per subscriber. A real policy-triggered incident has not yet been tested.Related PRs
main, so its diff currently includes this prerequisite. The fix is split out for separate review in Preserve Cloudflare connections and credential updates across sign-in #468.Follow-ups
Formatting
New files are formatted with
pnpm exec vp fmt. Existing files retain their surrounding formatting; whole-file formatting churn was removed to keep the review focused.