feat(webhooks): add webhook settings CRUD, envelope helpers, and recovery iterator#38
Merged
Conversation
Adds Zod schemas and TypeScript types for the GHIN webhook settings document (flat per-event-type maps for url, data_type, enabled), the delivery envelope, and the listing/recovery endpoints. Ships two standalone helpers usable without the client: - parseWebhookEnvelope<T>: validates an inbound delivery against the documented envelope shape and returns a typed Result. - verifyWebhookSignature / signWebhookPayload: HMAC-SHA256 with the sha256=<hex> prefix and timing-safe comparison. Header and algorithm are centralized so the scheme can be swapped from one place once USGA confirms the canonical format.
…ting
Adds six methods on GhinClient routed through RequestClient so they
inherit apiAccess login, automatic 401/403 re-login, the GHINcom
source header, and retry/backoff:
- webhooks.get / patch / delete — settings CRUD on the current user
- webhooks.test(type) — fires a synthetic event to the registered URL
- webhooks.list(req) — paged delivery history with status/object_type
filters; the basis for missed-delivery polling
- webhooks.resend({ webhook_id }) — replays a single delivery; defaults
is_crs_webhook=false
Adds playground/webhook-flow.ts driving the full flow end-to-end
against the sandbox.
Phase 3 of the webhook plan — two higher-level helpers on top of the
settings CRUD and listing endpoints.
- webhooks.ensureRegistered({ event, url, dataType?, enabled? })
GETs current settings and only PATCHes when the leaf for the given
event differs. Trailing-slash differences are treated as a match so
callers don't fire a spurious PATCH on every boot. Returns
{ changed, reason?, settings }.
- webhooks.iterateUndelivered({ object_type?, from_date?, to_date?,
per_page? }) → async generator yielding WebhookEnvelope. Pages
through status=not sent until a partial page signals exhaustion;
ideal foundation for missed-delivery recovery loops.
Also moves the plan from plans/todo/ to plans/done/.
- envelope: add 'crs' to object_type discriminator so CRS deliveries don't fail listing/iterator validation; settings keys stay at 6 - request-client: force %20 over '+' in query strings (JAX-RS and java.net.URI#getQuery don't decode '+' to space) - signature: accept Buffer/Uint8Array for raw bodies, not just string - settings: PATCH webhook_url validates as URL or empty string - ensureRegistered: print '(not set)' instead of literal 'undefined' - iterateUndelivered: max-pages safety cap (10k pages) to bound runaway recovery workers from a misconfigured filter
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR implements a complete webhook integration for the GHIN library, adding webhook settings CRUD operations, delivery history/replay, test endpoints, and low-level signing/verification utilities. The implementation spans data models, signature handling, client API surface, and supporting infrastructure. ChangesWebhook Integration for GHIN Library
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The PR introduces a substantial new API surface with 8 public methods, comprehensive validation schemas across 4 new model files, low-level signing/verification utilities, and 461 lines of test coverage. While individual pieces follow clear patterns (Zod validation, async/pagination, error handling), the breadth of the feature—spanning models, utilities, client integration, and tests—and the interplay between idempotent registration logic and async iteration require careful review of contract design, state comparison semantics, and pagination boundaries.
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
=======================================
Coverage ? 95.73%
=======================================
Files ? 53
Lines ? 2484
Branches ? 421
=======================================
Hits ? 2378
Misses ? 106
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/webhooks/signature.test.ts (1)
61-73: ⚡ Quick winAdd binary-body coverage for the
string | Buffer | Uint8Arraycontract.Current tests exercise string inputs only. Add Buffer/Uint8Array verification cases to lock in the expanded signature API behavior.
Suggested tests
describe('signWebhookPayload', () => { @@ it('changes when secret changes', () => { expect(signWebhookPayload(BODY, SECRET)).not.toBe(signWebhookPayload(BODY, 'other-secret')) }) + + it('verifies Buffer bodies', () => { + const body = Buffer.from(BODY) + const sig = signWebhookPayload(body, SECRET) + expect(verifyWebhookSignature(body, sig, SECRET)).toEqual({ ok: true }) + }) + + it('verifies Uint8Array bodies', () => { + const body = new Uint8Array(Buffer.from(BODY)) + const sig = signWebhookPayload(body, SECRET) + expect(verifyWebhookSignature(body, sig, SECRET)).toEqual({ ok: true }) + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/webhooks/signature.test.ts` around lines 61 - 73, Add tests in the signWebhookPayload suite to cover Buffer and Uint8Array inputs in addition to strings: for both a Buffer created from BODY and a Uint8Array derived from BODY, assert determinism (signWebhookPayload(BUF, SECRET) === signWebhookPayload(BUF, SECRET)), that changing the body bytes produces a different signature (e.g., altered Buffer/Uint8Array vs original), and that changing SECRET still changes the signature; use the existing BODY and SECRET constants and the signWebhookPayload function name to locate where to add these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/ghin/models/webhooks/listing.ts`:
- Line 12: The per_page field in the Zod schema inside
src/client/ghin/models/webhooks/listing.ts currently allows any positive
integer; update the schema for the per_page property to enforce the same upper
bound used elsewhere by adding a maximum of 100 (e.g., change the validator for
per_page to use .max(100) so it becomes an int, positive, default 25, and capped
at 100) to prevent oversized list requests from slipping through.
In `@src/client/ghin/models/webhooks/settings.ts`:
- Around line 38-42: The current .refine predicate only checks for presence of
top-level keys so inputs like { webhook_url: {} } pass; change the predicate
used in the .refine call to treat empty objects/empty strings as "not provided":
for value.webhook_url and value.webhook_data_type, if they are objects ensure
Object.keys(...) .length > 0 (and if strings ensure .trim().length > 0), and for
value.webhook_enabled keep the existing undefined check; update the predicate to
return true only if at least one of these is actually non-empty (non-empty
object/string or defined webhook_enabled) so no-op PATCH payloads with empty
event maps are rejected.
In `@src/playground/webhook-flow.ts`:
- Around line 16-23: The GhinClient is being instantiated outside the guarded
flow so constructor/config errors can escape the try/catch; move the new
GhinClient({...}) call into the try block inside fn and perform upfront
validation of required env vars (e.g., GHIN_PASSWORD, GHIN_USERNAME,
GHIN_API_ACCESS, GHIN_API_VERSION, GHIN_BASE_URL) before constructing; update
any other identical instantiation in the same file (the block around lines
31-60) to follow the same pattern so constructor errors are caught and logged by
the existing error handler.
In `@src/webhooks/parse-envelope.ts`:
- Line 20: The ValidationError currently includes the full inbound webhook
payload (seen where err(new ValidationError(`Invalid webhook envelope:
${parsed.error.message}`, undefined, undefined, payload))) which risks leaking
sensitive data; change this to stop passing the raw payload into the
ValidationError (e.g., remove the fourth argument or replace it with a minimal
non-sensitive marker), leaving the message using parsed.error.message and keep
the call site in parse-envelope.ts (the err(...) and ValidationError
construction) but do not attach the variable payload to the error object.
---
Nitpick comments:
In `@src/webhooks/signature.test.ts`:
- Around line 61-73: Add tests in the signWebhookPayload suite to cover Buffer
and Uint8Array inputs in addition to strings: for both a Buffer created from
BODY and a Uint8Array derived from BODY, assert determinism
(signWebhookPayload(BUF, SECRET) === signWebhookPayload(BUF, SECRET)), that
changing the body bytes produces a different signature (e.g., altered
Buffer/Uint8Array vs original), and that changing SECRET still changes the
signature; use the existing BODY and SECRET constants and the signWebhookPayload
function name to locate where to add these cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d771d70-b9c6-4038-bb2d-c9a2c24fc8d6
📒 Files selected for processing (17)
CLAUDE.mdplans/done/webhooks.mdsrc/client/ghin/index.test.tssrc/client/ghin/index.tssrc/client/ghin/models/index.tssrc/client/ghin/models/webhooks/envelope.tssrc/client/ghin/models/webhooks/index.tssrc/client/ghin/models/webhooks/listing.tssrc/client/ghin/models/webhooks/settings.tssrc/client/request-client/index.tssrc/index.tssrc/playground/webhook-flow.tssrc/webhooks/index.tssrc/webhooks/parse-envelope.test.tssrc/webhooks/parse-envelope.tssrc/webhooks/signature.test.tssrc/webhooks/signature.ts
Owner
Author
|
Addressed the outside-diff nitpick: added |
- listing: cap per_page at 100 to match iterateUndelivered schema
- settings: PATCH refine rejects empty event maps so {webhook_url: {}}
no longer slips through as a silent no-op
- parse-envelope: stop attaching raw webhook payload to ValidationError
(envelopes can carry PII — golfer IDs, scores, names)
- playground/webhook-flow: move GhinClient construction inside try/catch
- signature tests: add Buffer/Uint8Array coverage for the widened API
Owner
Author
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds first-class webhook support to
@spicygolf/ghinso callers don't have to re-implement login + raw fetch against/user/webhook_settings. Covers settings CRUD, delivery listing/replay, and verification helpers for inbound webhooks.Changes
Models (
src/client/ghin/models/webhooks/)settings.ts—WebhookSettings,WebhookSettingsPatch(URL-validated),EnsureRegisteredRequestenvelope.ts—WebhookEnvelope<T>,WebhookEnvelopeObjectType(the 6 settings event types plus'crs'for Course Rating System deliveries)listing.ts—WebhooksListRequest,WebhooksListResponse,WebhookResendRequest,IterateUndeliveredRequestInbound helpers (
src/webhooks/)parseWebhookEnvelope(payload)— Zod-validates the documented envelope shape, returns a typedResultsignWebhookPayload(rawBody, secret)/verifyWebhookSignature(rawBody, header, secret)— HMAC-SHA256, constant-time compare, acceptsstring | Buffer | Uint8ArrayClient namespace (
client.webhooks.*)get()/patch(settings)/delete()— settings CRUDtest(eventType)— fire a synthetic deliverylist(request?)/resend({ webhook_id, is_crs_webhook? })— listing + replayensureRegistered({ event, url, dataType?, enabled? })— idempotent register (GET-then-PATCH-only-if-different), normalizes trailing slashesiterateUndelivered({ object_type?, from_date?, ... })— async generator that pagesstatus=not sent, capped at 10k pagesPlumbing
RequestClientnow emits%20instead of+for spaces in query strings — JAX-RS /java.net.URI#getQuerystyle parsers don't decode+to space, and GHIN docs canonicalize as%20CLAUDE.md(unrelated drive-by)Assumptions to confirm with USGA
X-GHIN-Signature, valuesha256=<hex>. Constants exported so a confirmed-different scheme is a one-line change.payload.objectshape perobject_typeis not in Swagger.WebhookEnvelope<T>stays generic until real test-event bodies are captured.Testing
bun run test:run— 280 tests pass (envelope parser for eachobject_type, signature verifier matching/mismatched/length/missing, settings CRUD mocks, pagination exhaustion, ensureRegistered no-op/url-differs/enabled-differs/missing-leaf paths)./scripts/code-quality.sh— biome check, lint, build all greenbun run devagainstsrc/playground/webhook-flow.ts) — needsGHIN_WEBHOOK_URLenv var pointing at a public URL (e.g. webhook.site)Summary by CodeRabbit
New Features
Bug Fixes
+character handling issues.Documentation