Code Review Bench PR #26782 - fix: Make BOOKING_CANCELLED webhook payload consistent for regular cancellation and request-reschedule flow

[ketkarameya]

Code Review Bench PR calcom#26782 - fix: Make BOOKING_CANCELLED webhook payload consistent for regular cancellation and request-reschedule flow

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#11

fix: Make BOOKING_CANCELLED webhook payload consistent for regular cancellation and request-reschedule flow

---

Summary by Gitar

• New field:
  • requestReschedule boolean added to BOOKING_CANCELLED webhook payloads distinguishing regular cancellations (false) from reschedule-driven cancellations (true)
• Handler updates:
  • handleCancelBooking.ts sets requestReschedule: false for organizer-initiated cancellations; cancelAttendeeSeat.ts and requestReschedule.handler.ts set true for seat removals
• Type propagation:
  • Field added across BookingCancelledDTO, EventPayloadType, BookingCancelledParams, BookingExtraDataMap, and CancelledEventPayload for type safety
• Repository enhancement:
  • BookingRepository queries updated to fetch eventTypeId, title, length, and iCalSequence required for webhook payloads

_{This will update automatically on new commits.}

---

[gitar-bot-staging]

⚠️ Bug: Inconsistent default: requestReschedule ?? true vs ?? false

The BookingPayloadBuilder (v2021-10-20) defaults requestReschedule to true when the value is undefined/null:

requestReschedule: dto.requestReschedule ?? true,  // line 48

But the BookingWebhookFactory defaults it to false:

requestReschedule: params.requestReschedule ?? false,  // line 135

Per the PR's own semantics, false means "regular cancellation" and true means "reschedule-driven cancellation". The safe default should be false (regular cancellation is the more common case and the safer assumption). Currently, emitBookingCancelled isn't called for regular cancellations yet, but when it is wired up, any caller that omits requestReschedule will incorrectly tell consumers this is a reschedule-driven cancellation.

The default in BookingPayloadBuilder should be ?? false to match the BookingWebhookFactory and the documented behavior.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	requestReschedule: dto.requestReschedule ?? true,
	requestReschedule: dto.requestReschedule ?? false,

• Apply suggested fix

[gitar-bot-staging]

⚠️ Bug: Unrelated ENABLE_WEBHOOKS env var diverges from existing pattern

The new code introduces process.env.ENABLE_WEBHOOKS !== "false" as a webhook kill-switch in handleCancelBooking.ts. However:

1. Wrong variable name: The codebase already uses TASKER_ENABLE_WEBHOOKS (checked as === "1") in sendOrSchedulePayload.ts and WebhookService.ts. The new ENABLE_WEBHOOKS is a completely different, undocumented env variable with inverted check semantics.

2. Inconsistent application: The guard is only applied to handleCancelBooking.ts but NOT to the parallel cancelAttendeeSeat.ts code path (which also sends BOOKING_CANCELLED webhooks via sendPayload). This means the kill-switch only works for regular cancellations, not for seat-removal cancellations.

3. Out of scope: This kill-switch doesn't seem related to the PR's stated goal of adding requestReschedule to webhook payloads. It appears to be an unrelated change mixed in.

If a webhook kill-switch is truly needed, it should be implemented at the sendPayload level (so it applies consistently to all callers) and use a naming convention consistent with the existing TASKER_ENABLE_WEBHOOKS.

_{Was this helpful? React with 👍 / 👎}

• Apply suggested fix

[gitar-bot-staging]

⚠️ This PR is blocked due to unresolved code review findings.

_{Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.}

[gitar-bot-staging]

Code Review ⚠️ Changes requested 0 resolved / 2 findings

The requestReschedule field propagation is well-structured, but the BookingPayloadBuilder uses ?? true as the default while BookingWebhookFactory uses ?? false — the safe default for regular cancellations should be false in both places. Additionally, the ENABLE_WEBHOOKS env var is an unrelated, inconsistently applied addition that diverges from the existing TASKER_ENABLE_WEBHOOKS pattern.

⚠️ Bug: Inconsistent default: requestReschedule ?? true vs ?? false

📄 packages/features/webhooks/lib/factory/versioned/v2021-10-20/BookingPayloadBuilder.ts:48 📄 packages/lib/server/service/BookingWebhookFactory.ts:135

The BookingPayloadBuilder (v2021-10-20) defaults requestReschedule to true when the value is undefined/null:

requestReschedule: dto.requestReschedule ?? true,  // line 48

But the BookingWebhookFactory defaults it to false:

requestReschedule: params.requestReschedule ?? false,  // line 135

Per the PR's own semantics, false means "regular cancellation" and true means "reschedule-driven cancellation". The safe default should be false (regular cancellation is the more common case and the safer assumption). Currently, emitBookingCancelled isn't called for regular cancellations yet, but when it is wired up, any caller that omits requestReschedule will incorrectly tell consumers this is a reschedule-driven cancellation.

The default in BookingPayloadBuilder should be ?? false to match the BookingWebhookFactory and the documented behavior.

Suggested fix

            requestReschedule: dto.requestReschedule ?? false,

⚠️ Bug: Unrelated ENABLE_WEBHOOKS env var diverges from existing pattern

📄 packages/features/bookings/lib/handleCancelBooking.ts:414

The new code introduces process.env.ENABLE_WEBHOOKS !== "false" as a webhook kill-switch in handleCancelBooking.ts. However:

1. Wrong variable name: The codebase already uses TASKER_ENABLE_WEBHOOKS (checked as === "1") in sendOrSchedulePayload.ts and WebhookService.ts. The new ENABLE_WEBHOOKS is a completely different, undocumented env variable with inverted check semantics.

2. Inconsistent application: The guard is only applied to handleCancelBooking.ts but NOT to the parallel cancelAttendeeSeat.ts code path (which also sends BOOKING_CANCELLED webhooks via sendPayload). This means the kill-switch only works for regular cancellations, not for seat-removal cancellations.

3. Out of scope: This kill-switch doesn't seem related to the PR's stated goal of adding requestReschedule to webhook payloads. It appears to be an unrelated change mixed in.

If a webhook kill-switch is truly needed, it should be implemented at the sendPayload level (so it applies consistently to all callers) and use a naming convention consistent with the existing TASKER_ENABLE_WEBHOOKS.

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Enhanced minimal PR description with technical summary covering new requestReschedule field, handler updates, type propagation, and repository enhancements

_{5 rules not applicable. Show all rules by commenting gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[kayagokalp]

Benchmark Evaluation Results

Ground Truth Bugs (Recall: 2/4 = 50.0%)

ID	Severity	Title	Found?
GT-11-001	Important	requestReschedule field defaults to true instead of false, m	Yes
GT-11-002	Important	Attendee seat cancellation incorrectly sets requestReschedul	No
GT-11-003	Minor	ENABLE_WEBHOOKS environment variable only gates webhooks in	Yes
GT-11-004	Important	iCalSequence incremented in webhook payload but not synchron	No

Extra Gitar Findings

File	Line	Valid Bug?	Notes
(none)	-	-	-

Summary

• Recall: 50.0% (2/4)
• Precision: 100.0% (2/2)
• Precision counts ground truth matches + valid extra findings as true positives