Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71a9837576
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Adds spec-driven webhook event/delivery models plus framework-agnostic webhook receiver infrastructure to the TypeScript, Go, and Ruby SDKs, including routing (glob patterns), deduplication, middleware, and HMAC verification. This also extends existing generated models (e.g., Webhook.recent_deliveries, webhook payload fields on Person/Recording) so webhook payloads flow through codegen consistently.
Changes:
- Extend Smithy/OpenAPI models with
WebhookEvent,WebhookDelivery,WebhookCopy, andWebhook.recent_deliveries, plus webhook-payload fields onPersonandRecording. - Add TypeScript webhook receiver core + Node
httpadapter, signature helpers, and tests. - Add Go webhook receiver (
http.Handler), signature helpers, typed webhook event structs, parsing helpers, and tests; add Ruby receiver, Rack middleware adapter, signature verification, and tests.
Reviewed changes
Copilot reviewed 33 out of 39 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/tests/webhooks/verify.test.ts | Tests for TS HMAC signing/verification helpers. |
| typescript/tests/webhooks/handler.test.ts | Tests for TS receiver routing/dedup/middleware/signature parsing and fixtures. |
| typescript/tests/webhooks/events.test.ts | Tests for TS parseEventKind and constants. |
| typescript/tests/webhooks/adapters/node-http.test.ts | Tests for TS Node http adapter behavior. |
| typescript/tests/services/webhooks.test.ts | Validates generated service parsing for recent_deliveries. |
| typescript/src/webhooks/verify.ts | TS HMAC signing/verification implementation. |
| typescript/src/webhooks/index.ts | TS webhook module barrel exports. |
| typescript/src/webhooks/handler.ts | TS framework-agnostic receiver implementation. |
| typescript/src/webhooks/events.ts | TS generated-type re-exports + event kind parsing/constants. |
| typescript/src/webhooks/adapters/node-http.ts | TS Node http adapter for the receiver. |
| typescript/src/index.ts | Exposes webhook infrastructure from the package root exports. |
| typescript/src/generated/schema.d.ts | Generated TS types updated for webhook payloads and recent_deliveries. |
| typescript/src/generated/openapi-stripped.json | Generated OpenAPI subset updated with webhook shapes/fields. |
| typescript/src/generated/metadata.json | Regenerated SDK metadata timestamp. |
| spec/fixtures/webhooks/get.json | Adds recent_deliveries fixture with nested webhook request/response/event. |
| spec/fixtures/webhooks/event-unknown-future.json | Adds forward-compat webhook event fixture. |
| spec/fixtures/webhooks/event-todo-created.json | Adds todo webhook event fixture. |
| spec/fixtures/webhooks/event-message-copied.json | Adds message copy webhook event fixture with copy. |
| spec/basecamp.smithy | Adds webhook event/delivery/copy shapes and related model extensions. |
| ruby/test/basecamp/webhooks/verify_test.rb | Ruby tests for signature verification helper. |
| ruby/test/basecamp/webhooks/receiver_test.rb | Ruby tests for receiver routing/dedup/middleware/verification. |
| ruby/test/basecamp/webhooks/event_test.rb | Ruby tests for event wrapper parsing + kind parsing. |
| ruby/test/basecamp/services/webhooks_service_test.rb | Ruby generated service test updated for recent_deliveries. |
| ruby/lib/basecamp/webhooks/verify.rb | Ruby HMAC signing/verification helper implementation. |
| ruby/lib/basecamp/webhooks/receiver.rb | Ruby framework-agnostic receiver implementation. |
| ruby/lib/basecamp/webhooks/rack_middleware.rb | Ruby Rack adapter for webhook receiving. |
| ruby/lib/basecamp/webhooks/event.rb | Ruby event wrapper + kind parsing + recording type constants. |
| ruby/lib/basecamp/generated/types.rb | Regenerated Ruby types for new webhook fields/shapes. |
| ruby/lib/basecamp/generated/metadata.json | Regenerated Ruby metadata timestamp. |
| openapi.json | Root OpenAPI updated with webhook shapes/fields. |
| go/pkg/generated/client.gen.go | Regenerated Go models for webhook shapes/fields. |
| go/pkg/basecamp/webhooks_test.go | Go tests updated for recent_deliveries unmarshalling. |
| go/pkg/basecamp/webhooks.go | Go hand-written Webhook model updated + conversion from generated types. |
| go/pkg/basecamp/webhook_verify_test.go | Go tests for signature compute/verify helpers. |
| go/pkg/basecamp/webhook_verify.go | Go HMAC compute/verify helper implementation. |
| go/pkg/basecamp/webhook_handler_test.go | Go tests for receiver routing/dedup/middleware/verification/http handler. |
| go/pkg/basecamp/webhook_handler.go | Go receiver implementation (http.Handler) with routing/dedup/middleware/verification. |
| go/pkg/basecamp/webhook_event_test.go | Go tests for typed webhook event unmarshalling and kind parsing. |
| go/pkg/basecamp/webhook_event.go | Go typed webhook event structs + ParseEventKind + constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Model WebhookEvent, WebhookDelivery, and WebhookCopy types in the Smithy spec so they flow through the generation pipeline. Extend Person with can_ping/can_access_timesheet/can_access_hill_charts and Recording with content/comments_count/comments_url/subscription_url to match webhook payload shapes. Add recent_deliveries to Webhook for the GetWebhook response. Regenerate all downstream artifacts (OpenAPI, TS, Go, Ruby). Add shared webhook fixtures: event-todo-created, event-message-copied, event-unknown-future (forward-compat), and update get.json with recent_deliveries.
Hand-written webhook receiving layer that imports generated types: - WebhookReceiver with glob-pattern routing, event dedup, and middleware - HMAC-SHA256 signature verification (verify.ts) - Node.js HTTP adapter via createNodeHandler (adapters/node-http.ts) - Event kind parsing and WebhookEventKind constants (events.ts) - Public exports from src/index.ts All framework-agnostic: raw body + header accessor pattern. Adapters are thin wrappers for framework-specific integration.
Hand-written webhook receiving layer: - WebhookReceiver implementing http.Handler with glob-pattern routing, event dedup, and middleware chain - HMAC-SHA256 signature verification (webhook_verify.go) - WebhookEvent types with full recording/creator/copy modeling (webhook_event.go) - ParseEventKind for splitting "todo_created" into type + action - RecentDeliveries field on Webhook with mapping from generated types
Hand-written webhook receiving layer: - Basecamp::Webhooks::Receiver with glob-pattern routing, event dedup, and middleware chain - HMAC-SHA256 signature verification (verify.rb) - Event class with parsed_kind and RecordingType constants (event.rb) - RackMiddleware adapter for Rack-based frameworks - Autoloaded via Zeitwerk under Basecamp::Webhooks namespace
Address code review findings: 1. (High) Dedup only after successful handler execution — all three SDKs now call markSeen() after the middleware/handler chain completes without error. Transient handler failures no longer permanently suppress retries. 2. (High) Go MaxBodyBytes is now enforced as a hard limit. ServeHTTP reads MaxBodyBytes+1 bytes and returns 413 if the body exceeds the limit, instead of silently truncating via io.LimitReader. 3. (Medium) Go webhookPersonFromGenerated now maps all Person fields (title, bio, location, company, avatar_url, permissions flags, etc.) instead of only id/name/email_address. 4. (Medium) TS dedup uses string-based ID keys extracted from raw JSON via regex, avoiding Number precision loss on int64 event IDs. 5. (Low) Fix doc comments: "todo.*" → "todo_*" in TS and Ruby to match actual wildcard semantics. Adds regression tests for all five findings across all three SDKs.
- Go: TestWebhookPersonFromGenerated_AllFields asserts can_ping, company, title, bio, location, avatar_url, and all other Person fields survive the generated-to-hand-written conversion. - TS: Explicit test that duplicate of the exact same raw string ID is correctly deduped (complements the int64 precision test).
1. Replace two-phase isSeen/markSeen with atomic claim/commitSeen/ releaseClaim across all three SDKs. claim() checks both the seen set and a pending set, so concurrent requests for the same event ID cannot both pass the check. commitSeen() promotes from pending to seen on success; releaseClaim() removes from pending on error so retries work. 2. Replace TS regex ID extraction with a depth-aware JSON scanner that only matches "id" at brace depth 1 (top-level object), preventing nested recording.id or creator.id from being used as the dedup key. Adds concurrency regression tests (Go goroutines, TS Promise.allSettled, Ruby threads) and TS tests for nested-ID-before-top-level-ID scenarios.
- TS extractIdString: Full string-aware state machine at all depths. Braces inside JSON string values no longer corrupt depth tracking. - Go HandleRequest: Defer-based claim lifecycle so panics in handlers always release the pending claim, allowing retries. - TS node-http adapter: Configurable maxBodyBytes (default 1MB) with 413 response on oversized payloads. - Go Details field: Changed to `any` for forward compat with non-object JSON values. Tests updated to type-assert before indexing. - Go webhookEventFromGenerated: Only format CreatedAt when non-zero. - Go dispatchHandlers: Copy handlers under lock, release before executing.
Remove /buckets/{projectId} from TS and Ruby webhook test that
was added in PR #85 for the recent_deliveries fixture test.
Remove /buckets/{projectId} from TS and Ruby webhook test that
was added in PR #85 for the recent_deliveries fixture test.
Remove /buckets/{projectId} from TS and Ruby webhook test that
was added in PR #85 for the recent_deliveries fixture test.
Remove /buckets/{projectId} from TS and Ruby webhook test that
was added in PR #85 for the recent_deliveries fixture test.
Remove /buckets/{projectId} from TS and Ruby webhook test that
was added in PR #85 for the recent_deliveries fixture test.
* Flatten Smithy spec routes to remove nested URL scoping
Resources are uniquely identified and don't need parentage context in
their URLs. Remove /buckets/{projectId} prefix and other nested scoping
from ~80 operations, changing paths like:
/{accountId}/buckets/{projectId}/todos/{todoId}
to flat paths like:
/{accountId}/todos/{todoId}
Project-scoped operations under /projects/{projectId}/ are retained
since those genuinely operate on the project resource itself.
* Regenerate all SDK clients from flattened OpenAPI spec
* Update Go service layer for flat routes
Remove projectID parameter from all service methods that no longer
need bucket scoping. Retain projectID only for genuinely project-scoped
operations (ListProjectPeople, UpdateProjectAccess).
Also simplifies lineup marker requests to match spec (name/date only),
and removes BucketID from OperationInfo and OpenTelemetry spans.
* Update Go tests for flat route signatures
Remove projectID arguments from test calls, update lineup test
fixtures to use name/date fields, and remove BucketID from
observability test assertions.
* Update TypeScript tests for flat routes
Remove /buckets/{projectId} from MSW mock URLs, remove projectId
arguments from service calls, update lineup tests to use name/date
fields, and remove project_id from OpenTelemetry span assertions.
* Update Ruby tests for flat routes
Remove project_id: keyword arguments from service test calls, update
WebMock stub URLs to use flat paths, update lineup tests to use
name/date fields, and fix card step completion tests to use
set_card_step_completion on CardTablesService.
* Update conformance runner for flat routes
Remove projectId argument from CreateTodo and ListTodos calls in
the conformance test runner.
* Remove unused bucketID parameters from Go service methods
Nine methods still accepted a bucketID parameter that was silently
discarded — callers would pass a value that got thrown away. Remove
the parameter from: CampfiresService.GetLine, DeleteLine, GetChatbot,
UpdateChatbot, DeleteChatbot; ForwardsService.GetReply;
ClientRepliesService.Get; CardsService.Move; CardStepsService.Reposition.
Also remove dead ProjectPath() and RequireProject() methods from Client.
* Remove stale bucketID comments from Go service files
Clean 121 comment lines across 22 files that referenced bucketID
parameters that no longer exist in function signatures.
* Flatten conformance test paths to match new flat routes
Remove /buckets/{projectId} prefix from test path patterns and
projectId from pathParams in pagination, idempotency, and status-code
conformance tests. Update schema.json example accordingly.
* Update README examples and cleanup stale bucket references
Remove projectId arguments from Go, TypeScript, and Ruby README code
examples to match flattened service signatures. Update TS
normalizeUrlPath JSDoc comment. Remove dead bucket_path codegen logic
from Ruby service generator.
* Flatten timesheet entry operations added by PR #76
The rebase onto main brought in 3 new timesheet entry operations
(Get, Create, Update) with nested /buckets/{projectId}/ URIs.
Flatten these to match the rest of the flat branch, regenerate
all SDK clients, and update the Go service layer and conformance
runner accordingly.
* Remove dead hand-written TypeScript service files
These 31 files were superseded by generated services in
src/generated/services/ but never deleted. The entry point
(src/index.ts) already imports from generated, so these were
unused dead code. Only base.ts and authorization.ts remain
as hand-written services (they talk to different endpoints).
The ts-service branch will address generating a single unified
service layer that matches the ergonomics of these originals.
* Fix Ruby card_steps test and remove stale .plan.md
Update card_steps_service_test.rb to use the regenerated method
name (card_steps.set_completion instead of card_tables.set_card_step_completion).
Remove .plan.md that was accidentally included during rebase.
* Regenerate all SDK clients from canonical openapi.json
Post-rebase regeneration to ensure generated files match the
canonical openapi.json built from the Smithy spec.
* Flatten Boost API operations and regenerate all SDK clients
Flatten the 6 new Boost operation URIs from PR #83 to remove
/buckets/{projectId} nesting, matching the flat branch convention.
Regenerate all SDK clients and update tests for flat paths.
Add web-URL flattening to Go router so it can match Basecamp web URLs
(which still contain /buckets/{projectId}) against the flat route table.
* Flatten Go BoostsService and CreateLine for flat branch
Remove bucketID parameter from all 6 BoostsService methods and
update CreateLine test calls to match flat campfire signatures.
* Fix URL router missing .json-suffixed API URLs
The route table strips .json suffixes during generation, but the
router was matching against the raw path. API URLs like
basecampapi.com/.../boosts.json returned nil because the route
table stores .../boosts without the suffix.
Fix by normalizing .json in extractPath alongside other URL
normalization (scheme, host, query, trailing slash).
* Flatten webhook get-with-deliveries test URLs for flat branch
Remove /buckets/{projectId} from TS and Ruby webhook test that
was added in PR #85 for the recent_deliveries fixture test.
* Fix Go SDK build: wrap *int64 Id fields with derefInt64() in all converters
The required-fields PR changed all Id fields in generated types from
int64 to *int64. Update all *FromGenerated converter functions to use
derefInt64() for pointer-to-value conversion, and update test struct
literals to pass pointer values.
* Regenerate all SDK clients from Smithy spec and fix CI
Rebuild openapi.json from Smithy, then regenerate Ruby, TypeScript,
and Kotlin SDKs. Updates Kotlin conformance tests and TodosServiceTest
for flat URL signatures (projectId removed). Fixes apidiff CI cache
key to include go.mod hash so the tool rebuilds on Go version bumps.
* Fix spec routes C1, C2, C5, C6 and regenerate all SDKs
C1: GetProjectTimeline now bucket-scoped at /buckets/{projectId}/timeline.json
C2: GetProjectTimesheet now bucket-scoped at /buckets/{projectId}/timesheet.json
C5: Timesheet entry routes use /timesheet_entries/{entryId} (underscore separator)
C6: ListWebhooks/CreateWebhook now bucket-scoped at /buckets/{bucketId}/webhooks.json
Propagated across Go (generated + hand-written), Ruby, TypeScript, Kotlin, Swift.
Go URL router handles /projects/ → /buckets/ normalization for web URLs.
TS normalizeUrlPath uses context overrides for bucketId vs projectId.
Conformance tests cover all four route fixes (Go 31/31, Kotlin 30/30+1 skip).
* Fix PR review comments: update stale docs and comments
- Go README: Add bucketID to webhook Create/List examples
- Go README: Define boardID variable in message board example
- timesheet.go: Remove stale projectID reference from RecordingReport comment
- message_types.go: Update List comment to reflect account-level scope
* Fix timeline/timesheet paths: /buckets/ → /projects/
BC3 serves GetProjectTimeline and GetProjectTimesheet at
/projects/{projectId}/... not /buckets/{projectId}/...
Main branch already had this fix but flat never picked it up.
* Update conformance and SDK tests for /projects/ timeline/timesheet paths
Summary
WebhookEvent,WebhookDelivery, andWebhookCopytypes in Smithy so they flow through the generation pipeline. ExtendPersonandRecordingwith fields present in webhook payloads. Addrecent_deliveriestoWebhook.WebhookReceiverwith glob-pattern routing, event dedup, middleware chain, HMAC-SHA256 verification, andcreateNodeHandleradapter for Node.js HTTP servers.WebhookReceiverimplementinghttp.Handlerwith the same routing/dedup/middleware/verification pattern, plusParseEventKindand typed event structs.Basecamp::Webhooks::Receiverwith glob routing, dedup, middleware, HMAC verification, andRackMiddlewareadapter for Rack-based frameworks.All three receivers are framework-agnostic at their core (raw body + header accessor), with thin adapters for framework-specific integration. Forward-compatible: unknown event kinds and recording types are accepted without error.
Design
The webhook event payload types are defined in the Smithy spec and flow through the generation pipeline (
GetWebhook→Webhook→recent_deliveries→WebhookDelivery→request.body: WebhookEvent). The hand-written receiver infrastructure imports generated types, keeping the spec as the single source of truth.Key features across all three SDKs:
on("todo_*", handler)matches all todo events(event, next) → ...for pluggable preprocessingDocument/unknownfor free-form details, unknown event kinds route to catch-all handlersTest plan
makepasses clean (all three SDKs build, test, lint)event-unknown-future.json) parsed without error by all three SDKs