Add factory webhook handler#360
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 27 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a webhook handling and registration system along with an in-memory state store in the factory-sdk package. Specifically, it adds a WebhookHandler for processing and verifying signatures of incoming events (Linear, Slack, GitHub), a WebhookRegistrar for managing subscriptions, and an InMemoryStateStore implementing a StateStore interface. The review feedback highlights several critical improvements: preventing a memory leak in the webhook deduplication cache, addressing potential timing leaks and decoding vulnerabilities in the signature verification function, validating incoming payloads to avoid runtime type errors, and adding a guard to prevent orphaned webhook subscriptions when registering multiple times.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (eventId) { | ||
| this.#seen.add(eventId) | ||
| } |
There was a problem hiding this comment.
The #seen Set is used to deduplicate webhook events by eventId. However, because it is an unbounded Set, it will grow indefinitely as more events are processed. In a long-running server process, this will eventually lead to a memory leak and potential Out Of Memory (OOM) crashes.
To prevent this, we can limit the size of the Set by evicting the oldest entries (FIFO) when a threshold (e.g., 10,000 events) is exceeded.
if (eventId) {
this.#seen.add(eventId)
if (this.#seen.size > 10000) {
const oldest = this.#seen.values().next().value
if (oldest !== undefined) {
this.#seen.delete(oldest)
}
}
}| function timingSafeHexEqual(a: string, b: string): boolean { | ||
| if (a.length !== b.length) return false | ||
| const bufA = Buffer.from(a, 'hex') | ||
| const bufB = Buffer.from(b, 'hex') | ||
| return bufA.length === bufB.length && cryptoTimingSafeEqual(bufA, bufB) | ||
| } |
There was a problem hiding this comment.
The current implementation of timingSafeHexEqual has potential timing leaks and vulnerabilities:
- Length-based Timing Leak: If
a.length !== b.length, it returnsfalseimmediately, leaking the length of the expected signature. - Hex Decoding Anomalies:
Buffer.from(b, 'hex')can produce buffers of different lengths ifbcontains non-hex characters (e.g.,Buffer.from('zz', 'hex')has length 0). This causesbufA.length === bufB.lengthto short-circuit, leaking whether the input contains valid hex characters.
To resolve these issues, we can compare the strings as UTF-8 buffers (since hex characters are safe ASCII/UTF-8). If the lengths differ, we can perform a dummy comparison against bufA itself to ensure constant-time execution before returning false.
| function timingSafeHexEqual(a: string, b: string): boolean { | |
| if (a.length !== b.length) return false | |
| const bufA = Buffer.from(a, 'hex') | |
| const bufB = Buffer.from(b, 'hex') | |
| return bufA.length === bufB.length && cryptoTimingSafeEqual(bufA, bufB) | |
| } | |
| function timingSafeHexEqual(a: string, b: string): boolean { | |
| const bufA = Buffer.from(a, 'utf8') | |
| const bufB = Buffer.from(b, 'utf8') | |
| if (bufA.length !== bufB.length) { | |
| cryptoTimingSafeEqual(bufA, bufA) | |
| return false | |
| } | |
| return cryptoTimingSafeEqual(bufA, bufB) | |
| } |
| let event: WebhookEvent | ||
| try { | ||
| event = JSON.parse(rawBody) as WebhookEvent | ||
| } catch { | ||
| return new Response('bad_request', { status: 400 }) | ||
| } |
There was a problem hiding this comment.
The parsed event is cast directly to WebhookEvent without validation. If the incoming payload is malformed or missing the path property, event.path will be undefined. This will cause event.path.startsWith(...) to throw a TypeError, resulting in a 500 internal_error response.
To make the handler more robust and return a proper 400 bad_request for malformed payloads, we should validate that event?.path is a string before proceeding.
let event: WebhookEvent
try {
event = JSON.parse(rawBody) as WebhookEvent
} catch {
return new Response('bad_request', { status: 400 })
}
if (typeof event?.path !== 'string') {
return new Response('bad_request', { status: 400 })
}| async register(): Promise<void> { | ||
| const pathGlobs = this.#config.pathGlobs ?? [ |
There was a problem hiding this comment.
If register() is called multiple times on the same registrar instance, it will overwrite this.#subscriptionId without unregistering or throwing an error. This can lead to orphaned webhook subscriptions on the RelayFile server.
We should add a guard to prevent registering again if a subscription is already active.
| async register(): Promise<void> { | |
| const pathGlobs = this.#config.pathGlobs ?? [ | |
| async register(): Promise<void> { | |
| if (this.#subscriptionId) { | |
| throw new Error('Webhook is already registered') | |
| } | |
| const pathGlobs = this.#config.pathGlobs ?? [ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3401544ae5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (eventId && this.#seen.has(eventId)) { | ||
| return new Response('ok', { status: 200 }) | ||
| } |
There was a problem hiding this comment.
When two deliveries with the same x-relay-event-id arrive concurrently, both requests can pass this #seen check before either handler finishes and adds the id at the end, so both provider handlers run and can spawn duplicate factory work or duplicate writebacks. Duplicate delivery is a normal Relayfile failure mode; reserve the event id or keep a keyed in-flight promise before invoking the provider handler, and clear it on retryable failure so later retries still work.
Useful? React with 👍 / 👎.
| const subscription = await this.#config.client.registerWebhook({ | ||
| workspaceId: this.#config.workspaceId, | ||
| url: this.#config.receiverUrl, | ||
| pathGlobs, | ||
| secret: this.#config.secret, | ||
| }) | ||
| this.#subscriptionId = subscription.subscriptionId |
There was a problem hiding this comment.
Calling register() more than once creates another webhook subscription and overwrites the only stored #subscriptionId; unregister() can then delete only the newest subscription, leaving the earlier one active and causing duplicate webhook deliveries for the same factory. This can happen on retries/reconnects or repeated startup paths, so register() should be idempotent and preferably coalesce concurrent calls before invoking registerWebhook.
Useful? React with 👍 / 👎.
|
Findings
Addressed Comments
Verification No files were edited. Ran:
I did not run the macOS-only packaging smoke job locally. |
|
Findings
Addressed comments
Verification No files were edited. Ran successfully:
GitHub reports PR #360 is already merged/closed, so I am not printing |
Merge origin/main (#360 webhook handler, #362 park completed PRs). The ar-366 re-dispatch and ar-243 PR-state sweep tests came from main with pre-label-routing implementer names; under AR-274 label routing the implementer is repo-slug suffixed (ar-366-impl-pear / ar-243-impl-pear). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* AR-274 route implementers by repo labels * fix(factory): fail dispatch without repo labels * AR-274 rebind reviewer to label route * AR-274 require repo labels for dispatch (no default-route fallback) Remove defaultRouteAssignment/repoSlug so an unlabeled or unmapped issue fails dispatch loudly instead of silently routing to repos.default. Cap label implementers at MAX_LABEL_IMPLEMENTERS=4 and rebind the reviewer to the first label route via routeReviewerSpec(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(factory): green CI + dedupe invalid-label dispatch comments - fleet.test.ts: implementer name is now repo-slug suffixed (ar-77-impl-pear) under label routing. - Dedupe the invalid-label dispatch failure comment by reason+offending-labels signature so a stuck Ready issue (or the writeback's own change event) no longer re-posts the same notice every cycle; clear on successful dispatch. Addresses Codex review on PR #363. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(factory): suffix implementer names for merged-in main tests Merge origin/main (#360 webhook handler, #362 park completed PRs). The ar-366 re-dispatch and ar-243 PR-state sweep tests came from main with pre-label-routing implementer names; under AR-274 label routing the implementer is repo-slug suffixed (ar-366-impl-pear / ar-243-impl-pear). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Tests