Code Review Bench PR #26523 - chore: Integrate confirmation booking audit

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#10

chore: Integrate confirmation booking audit

---

Summary by Gitar

Fixed duplicate booking acceptance events: Recurring booking confirmations triggered fireBookingAcceptedEvent twice (once in recurring block, once at handler end). Now fires early in recurring block with conditional if (!recurringEventId) check at handler end to prevent duplicates.

Added organization context resolution: Audit events for recurring bookings lacked org/team scope. Now resolves teamId via getTeamIdFromEventType, determines user-level vs org-level firing based on team existence and parent relationships, and includes organizationId in event payload.

Corrected audit log status accuracy: Audit logs recorded stale pre-update status values. Now queries database immediately after updateMany calls (e.g., for rejections) to capture actual persisted REJECTED status instead of in-memory stale data.

Implemented multi-tenant event scoping: Events fire at user-level when no team exists or for sub-teams (parentId present), and at org-level for top-level teams. Uses getOrgIdFromMemberOrTeamId to resolve organization context.

_{This will update automatically on new commits.}

---

[gitar-bot-staging]

⚠️ Bug: Non-recurring accepted booking audit logs ACCEPTED as oldStatus

In the non-recurring booking acceptance path in handleConfirmation.ts, oldStatus is hardcoded to BookingStatus.ACCEPTED (line 413). This is incorrect — handleConfirmation is called to confirm a booking, meaning the booking's prior status is typically PENDING (for bookings requiring confirmation). The booking.status field is available in scope and should be used instead.

This causes the audit log to record a status transition of ACCEPTED → ACCEPTED, which is meaningless and defeats the purpose of the status tracking in audit logs.

The recurring path correctly captures oldStatus from unconfirmedRecurringBookings before the update, so the fix is straightforward.

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

Suggested change

	acceptedBookings = [
	acceptedBookings = [
	{
	oldStatus: booking.status,
	uid: booking.uid,
	},
	];

• Apply suggested fix

[gitar-bot-staging]

⚠️ Bug: Recurring rejection reads post-update status as oldStatus

In confirm.handler.ts, the recurring booking rejection path first calls updateMany to set status to REJECTED (line 420-430), then queries the database to get the "fresh" status (line 432-442), and uses that as oldStatus (line 444-446).

Since the query runs after the update, recurringBooking.status will be REJECTED — the new status, not the old one. This means the audit log records a transition of REJECTED → REJECTED, which is wrong. It should record PENDING → REJECTED.

The PR description mentions the intent was to "query database immediately after status updates to get fresh values for audit logging," but for oldStatus the pre-update value is what's needed. The unconfirmedRecurringBookings query (line 409-418) already captures the pre-update status (which should be PENDING). Use that instead.

Suggested approach: map oldStatus from unconfirmedRecurringBookings instead of updatedRecurringBookings.

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

Suggested change

	const updatedRecurringBookings = await prisma.booking.findMany({
	rejectedBookings = unconfirmedRecurringBookings.map((recurringBooking) => ({
	uid: recurringBooking.uid,
	oldStatus: recurringBooking.status,
	}));

• 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

Two bugs in audit status tracking: non-recurring acceptance hardcodes ACCEPTED as oldStatus instead of using booking.status, and recurring rejection reads the post-update REJECTED status from DB and records it as oldStatus. Both produce incorrect old → new transitions in audit logs.

⚠️ Bug: Non-recurring accepted booking audit logs ACCEPTED as oldStatus

📄 packages/features/bookings/lib/handleConfirmation.ts:411

In the non-recurring booking acceptance path in handleConfirmation.ts, oldStatus is hardcoded to BookingStatus.ACCEPTED (line 413). This is incorrect — handleConfirmation is called to confirm a booking, meaning the booking's prior status is typically PENDING (for bookings requiring confirmation). The booking.status field is available in scope and should be used instead.

This causes the audit log to record a status transition of ACCEPTED → ACCEPTED, which is meaningless and defeats the purpose of the status tracking in audit logs.

The recurring path correctly captures oldStatus from unconfirmedRecurringBookings before the update, so the fix is straightforward.

Suggested fix

    acceptedBookings = [
      {
        oldStatus: booking.status,
        uid: booking.uid,
      },
    ];

⚠️ Bug: Recurring rejection reads post-update status as oldStatus

📄 packages/trpc/server/routers/viewer/bookings/confirm.handler.ts:432

In confirm.handler.ts, the recurring booking rejection path first calls updateMany to set status to REJECTED (line 420-430), then queries the database to get the "fresh" status (line 432-442), and uses that as oldStatus (line 444-446).

Since the query runs after the update, recurringBooking.status will be REJECTED — the new status, not the old one. This means the audit log records a transition of REJECTED → REJECTED, which is wrong. It should record PENDING → REJECTED.

The PR description mentions the intent was to "query database immediately after status updates to get fresh values for audit logging," but for oldStatus the pre-update value is what's needed. The unconfirmedRecurringBookings query (line 409-418) already captures the pre-update status (which should be PENDING). Use that instead.

Suggested approach: map oldStatus from unconfirmedRecurringBookings instead of updatedRecurringBookings.

Suggested fix

      rejectedBookings = unconfirmedRecurringBookings.map((recurringBooking) => ({
        uid: recurringBooking.uid,
        oldStatus: recurringBooking.status,
      }));

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Appended 'Summary by Gitar' section with 4 technical bullet points covering duplicate event fixes, organization context resolution, status data accuracy, and multi-tenant scoping

_{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-10-001	Important	Single booking acceptance audit logs hardcoded oldStatus as	Yes
GT-10-002	Important	Recurring booking rejection audit logs use post-update statu	Yes
GT-10-003	Important	Breaking schema change to rejectionReason field without vers	No
GT-10-004	Important	Audit event fired before database updates complete, creating	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