Code Review Bench PR #25771 - feat: update recording, transcript endpoint and add tests

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#16

feat: update recording, transcript endpoint and add tests

---

Summary by Gitar

• Security enhancement:
  • Enabled three-layer guard chain (ApiAuthGuard, BookingUidGuard, BookingPbacGuard) for recordings and transcripts endpoints
• Permission update:
  • Changed recordings endpoint from BOOKING_READ to BOOKING_WRITE for elevated access control
• Tests activated:
  • Uncommented 48 lines of e2e authorization tests verifying 200 for authorized users and 403 for unauthorized access

_{This will update automatically on new commits.}

---

[gitar-bot-staging]

🚨 Bug: Missing await on async getRecordings() returns a Promise

CalVideoService.getRecordings() is an async method that returns Promise<RecordingItem[]> (it internally awaits database queries and Promise.all). The await was removed from the call in the controller, so recordings is now a Promise object, not the resolved array.

The return type annotation says Promise<GetBookingRecordingsOutput> with data: RecordingItem[], but the actual data field will contain an unresolved Promise. This will result in the API returning "data": {} (a serialized Promise object) instead of the actual recordings array, effectively breaking the recordings endpoint for all callers.

The test mocks this with mockResolvedValue([]) which returns a Promise — and since the handler is async, the returned object's data field will be a Promise that NestJS may or may not auto-resolve depending on serialization timing, but TypeScript's type system is being circumvented here.

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

Suggested change

	const recordings = this.calVideoService.getRecordings(bookingUid);
	const recordings = await this.calVideoService.getRecordings(bookingUid);

• Apply suggested fix

[gitar-bot-staging]

⚠️ Security: Guard order on transcripts puts PBAC before authentication

The transcripts endpoint uses @UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard), which places BookingPbacGuard before ApiAuthGuard. NestJS executes guards in the order they are listed. This means the PBAC guard — which checks whether the authenticated user has access to the booking — will run before the user is even authenticated.

Every other endpoint in this controller that uses these three guards uses the order ApiAuthGuard, BookingUidGuard, BookingPbacGuard (authenticate → validate UID → check authorization). The recordings endpoint on line 217 uses this correct order. The transcripts endpoint should match.

Depending on the BookingPbacGuard implementation, this could either:

• Crash/throw when trying to access the user context (which hasn't been set yet)
• Or accidentally allow unauthenticated access if the guard fails open when no user is present

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

Suggested change

	@UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard)
	@UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard)

• 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 🚫 Blocked 0 resolved / 3 findings

Security hardening intent is good, but the recordings endpoint has a critical missing await that will return a Promise object instead of actual data, and the transcripts guard ordering puts authorization before authentication.

🚨 Bug: Missing await on async getRecordings() returns a Promise

📄 apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts:227

CalVideoService.getRecordings() is an async method that returns Promise<RecordingItem[]> (it internally awaits database queries and Promise.all). The await was removed from the call in the controller, so recordings is now a Promise object, not the resolved array.

The return type annotation says Promise<GetBookingRecordingsOutput> with data: RecordingItem[], but the actual data field will contain an unresolved Promise. This will result in the API returning "data": {} (a serialized Promise object) instead of the actual recordings array, effectively breaking the recordings endpoint for all callers.

The test mocks this with mockResolvedValue([]) which returns a Promise — and since the handler is async, the returned object's data field will be a Promise that NestJS may or may not auto-resolve depending on serialization timing, but TypeScript's type system is being circumvented here.

Suggested fix

    const recordings = await this.calVideoService.getRecordings(bookingUid);

⚠️ Security: Guard order on transcripts puts PBAC before authentication

📄 apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts:238

The transcripts endpoint uses @UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard), which places BookingPbacGuard before ApiAuthGuard. NestJS executes guards in the order they are listed. This means the PBAC guard — which checks whether the authenticated user has access to the booking — will run before the user is even authenticated.

Every other endpoint in this controller that uses these three guards uses the order ApiAuthGuard, BookingUidGuard, BookingPbacGuard (authenticate → validate UID → check authorization). The recordings endpoint on line 217 uses this correct order. The transcripts endpoint should match.

Depending on the BookingPbacGuard implementation, this could either:

• Crash/throw when trying to access the user context (which hasn't been set yet)
• Or accidentally allow unauthenticated access if the guard fails open when no user is present

Suggested fix

  @UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard)

💡 Quality: Unused BookingRepository type import added

📄 packages/platform/types/bookings/2024-08-13/outputs/get-booking-recordings.output.ts:6

A new import type { BookingRepository } was added to get-booking-recordings.output.ts but is never used anywhere in the file. This appears to be an accidental addition and should be removed to keep the file clean.

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Appended technical summary with 3 categories (security enhancement, permission update, tests activated) to minimal PR description

_{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}