fix(GRO-624): input validation across 5 API routes #305

Merged

groombook-engineer[bot] merged 0 commits from fix/gro-624-input-validation into dev 2026-04-16 12:34:57 +00:00

Conversation 4 Commits - Files Changed -

groombook-engineer[bot] commented 2026-04-16 12:25:11 +00:00 (Migrated from github.com)

Copy Link

Copy Source

Summary

• fix(invoices): add Zod query param validation to GET / (clientId, appointmentId, status, limit, offset)
• fix(book): add future-time refinement to booking startTime
• fix(appointments): cap recurrence series at 1 year max (frequencyWeeks × count ≤ 52)
• fix(services): cap durationMinutes at 480 (8 hours max)
• fix(stripe-webhooks): validate invoice IDs as UUIDs before DB lookup

Test plan

• Run existing tests: pnpm --filter api test
• Verify all 5 routes accept valid input and reject invalid input

cc @cpfarhood

## Summary - fix(invoices): add Zod query param validation to GET / (clientId, appointmentId, status, limit, offset) - fix(book): add future-time refinement to booking startTime - fix(appointments): cap recurrence series at 1 year max (frequencyWeeks × count ≤ 52) - fix(services): cap durationMinutes at 480 (8 hours max) - fix(stripe-webhooks): validate invoice IDs as UUIDs before DB lookup ## Test plan - [ ] Run existing tests: `pnpm --filter api test` - [ ] Verify all 5 routes accept valid input and reject invalid input cc @cpfarhood

👍 1

lint-roller-qa[bot] (Migrated from github.com) approved these changes 2026-04-16 12:27:26 +00:00

greptile-apps[bot] commented 2026-04-16 12:31:08 +00:00 (Migrated from github.com)

Copy Link

Copy Source

Greptile Summary

This PR adds the five targeted input validations described in the PR description (invoice query params, booking future-time check, recurrence 1-year cap, 8-hour service duration cap, and Stripe UUID guard), all of which are correct. However, the diff includes a much wider set of behavioral changes that are not mentioned in the description, and several of them introduce regressions:

• P0 — data loss (groomingLogs.ts): appointmentId is destructured out of the request body but never included in the DB insert. Every grooming log created after this lands will have a null appointment link regardless of what the caller provides.
• P0 — double-refund risk (invoices.ts): The refund endpoint lost its idempotency-key guard and transaction-backed refunds table insert. A network retry or duplicate request will call processRefund twice, resulting in two Stripe refunds.
• P1 — invalid invoice transitions (invoices.ts): The ALLOWED_TRANSITIONS state machine was replaced with a void-only check. Paid invoices can now be moved back to draft or pending through the API.
• P1 — bather-only PATCH skips conflict check (appointments.ts): updateFields.batherStaffId was removed from needsConflictCheck, so a PATCH that only changes the bather staff member bypasses the scheduling conflict block entirely.
• P1 — cascade update has no conflict checking (appointments.ts): The per-appointment staffId/batherStaffId conflict queries were removed from the cascade update loop, allowing a cascaded reschedule to silently double-book a groomer across an entire series.
• P1 — breaking API shape change (invoices.ts): POST /:id/tip-splits now returns a bare array instead of the enriched invoice object, which will break any client expecting the previous shape.

Confidence Score: 1/5

Not safe to merge — two P0 bugs (data loss on grooming logs, double-refund risk) and three P1 regressions need to be resolved first.

The five advertised validations are all correct, but the PR bundles a wide set of undescribed behavioral changes. Two are critical: appointmentId is permanently dropped from every new grooming log (silent data corruption), and the refund endpoint lost its idempotency guard (financial double-charge risk on any retry). Three further P1 bugs — invalid invoice status transitions, bather-only PATCH bypassing conflict checks, and cascade updates losing all conflict detection — add scheduling reliability concerns. The PR needs targeted fixes before landing.

apps/api/src/routes/groomingLogs.ts (appointmentId drop), apps/api/src/routes/invoices.ts (refund safety, status transitions, tip-splits response), apps/api/src/routes/appointments.ts (cascade and bather conflict checks)

Important Files Changed

Filename	Overview
apps/api/src/routes/groomingLogs.ts	P0 data-loss bug: appointmentId is destructured out of the body but never re-added to the DB insert, so every new grooming log is silently stored with a null appointment link.
apps/api/src/routes/invoices.ts	Three regressions: (1) refund idempotency key and audit-trail transaction removed (double-refund risk); (2) invoice status state-machine replaced with void-only guard (paid→draft now allowed); (3) tip-splits response shape changed from enriched invoice object to bare array (breaking API change).
apps/api/src/routes/appointments.ts	Two conflict-check regressions: cascade update loop lost all staffId/batherStaffId conflict queries; batherStaffId-only PATCH bypasses the conflict check block entirely. The new recurrence 1-year cap (frequencyWeeks × count ≤ 52) is correct.
apps/api/src/routes/stripe-webhooks.ts	UUID guard added correctly; minor: parsed.data unused, invoiceId.trim() called twice instead.
apps/api/src/routes/services.ts	Clean addition of .max(480) on durationMinutes; no issues.
apps/api/src/routes/book.ts	Future-time Zod refinement on startTime is correct. Confirm/cancel token handlers simplified from atomic WHERE clause to ID-based update with pre-checks; idempotent in practice but introduces a TOCTOU window.
apps/api/src/routes/appointmentGroups.ts	Refactoring of groomer access checks into hasAccess intermediate variable; logic preserved. One removed if (!updated) return 404 after an already-checked existence query — benign.

Sequence Diagram

sequenceDiagram
    participant Client
    participant API
    participant DB
    participant Stripe

    Note over Client,Stripe: Refund flow — BEFORE this PR (safe)
    Client->>API: POST /invoices/:id/refund {amountCents, idempotencyKey}
    API->>DB: BEGIN TRANSACTION
    DB->>DB: SELECT refunds WHERE idempotencyKey = ?
    alt Duplicate request
        DB-->>API: existing refund found
        API-->>Client: 200 {refundId} (idempotent)
    else First request
        API->>Stripe: processRefund()
        Stripe-->>API: {refundId}
        API->>DB: INSERT refunds (audit trail)
        DB-->>API: OK
        API->>DB: COMMIT
        API-->>Client: 200 {refundId}
    end

    Note over Client,Stripe: Refund flow — AFTER this PR (risk)
    Client->>API: POST /invoices/:id/refund {amountCents}
    API->>Stripe: processRefund()
    Stripe-->>API: {refundId}
    API-->>Client: 200 {refundId}
    Note over Client,Stripe: Retry → second processRefund() call → double charge

Comments Outside Diff (6)

1. apps/api/src/routes/groomingLogs.ts, line 59-105 (link)

   appointmentId silently dropped from every new grooming log

   appointmentId is destructured out of rest on line 59 but is never re-added to the .values(...) call (lines 99–105). Every new grooming visit log will be stored with a null appointment linkage, regardless of what the caller supplies. The original code passed it explicitly as appointmentId: appointmentId ?? null.

   Fix: re-include appointmentId in the insert:

   const [row] = await db
     .insert(groomingVisitLogs)
     .values({
       ...rest,
       appointmentId: appointmentId ?? null,
       groomedAt: groomedAt ? new Date(groomedAt) : new Date(),
     })
     .returning();
   

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/api/src/routes/groomingLogs.ts
   Line: 59-105
   
   Comment:
   **`appointmentId` silently dropped from every new grooming log**
   
   `appointmentId` is destructured out of `rest` on line 59 but is never re-added to the `.values(...)` call (lines 99–105). Every new grooming visit log will be stored with a `null` appointment linkage, regardless of what the caller supplies. The original code passed it explicitly as `appointmentId: appointmentId ?? null`.
   
   Fix: re-include `appointmentId` in the insert:
   ```ts
   const [row] = await db
     .insert(groomingVisitLogs)
     .values({
       ...rest,
       appointmentId: appointmentId ?? null,
       groomedAt: groomedAt ? new Date(groomedAt) : new Date(),
     })
     .returning();
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.
   

2. apps/api/src/routes/invoices.ts, line 382-385 (link)

   Refund idempotency and audit trail removed — double-charge risk

   The previous implementation:

   1. Accepted an idempotencyKey and short-circuited on duplicates (preventing network-retry double refunds).
   2. Persisted every refund record to the refunds table inside a transaction for audit purposes.

   Both safety mechanisms were removed. Now a transient network error or client retry will call processRefund twice, issuing two refunds to Stripe. There is also no database record of issued refunds.

   Consider restoring at minimum the idempotency guard — even a simple refunds table insert with an ON CONFLICT DO NOTHING on stripeRefundId provides a meaningful safety net.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/api/src/routes/invoices.ts
   Line: 382-385
   
   Comment:
   **Refund idempotency and audit trail removed — double-charge risk**
   
   The previous implementation:
   1. Accepted an `idempotencyKey` and short-circuited on duplicates (preventing network-retry double refunds).
   2. Persisted every refund record to the `refunds` table inside a transaction for audit purposes.
   
   Both safety mechanisms were removed. Now a transient network error or client retry will call `processRefund` twice, issuing two refunds to Stripe. There is also no database record of issued refunds.
   
   Consider restoring at minimum the idempotency guard — even a simple `refunds` table insert with an `ON CONFLICT DO NOTHING` on `stripeRefundId` provides a meaningful safety net.
   
   How can I resolve this? If you propose a fix, please make it concise.
   

3. apps/api/src/routes/invoices.ts, line 318-320 (link)

   Status state-machine removed — invalid transitions now accepted

   The ALLOWED_TRANSITIONS guard was replaced with only a void check, so the following transitions are now silently accepted by the API:

   • paid → draft / paid → pending (un-paying a settled invoice)
   • pending → draft (already-sent invoice rolled back to draft)

   The previous state machine modelled real business constraints. At a minimum, paid invoices should not be moveable to draft or pending, which could create incorrect financial state.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/api/src/routes/invoices.ts
   Line: 318-320
   
   Comment:
   **Status state-machine removed — invalid transitions now accepted**
   
   The `ALLOWED_TRANSITIONS` guard was replaced with only a void check, so the following transitions are now silently accepted by the API:
   
   - `paid → draft` / `paid → pending` (un-paying a settled invoice)
   - `pending → draft` (already-sent invoice rolled back to draft)
   
   The previous state machine modelled real business constraints. At a minimum, `paid` invoices should not be moveable to `draft` or `pending`, which could create incorrect financial state.
   
   How can I resolve this? If you propose a fix, please make it concise.
   

4. apps/api/src/routes/appointments.ts, line 424-428 (link)

   Bather-only PATCH skips conflict check entirely

   updateFields.batherStaffId !== undefined was removed from needsConflictCheck. When a client sends a PATCH that only changes the bather (e.g. { "batherStaffId": "uuid" }), needsConflictCheck is false and the entire conflict-check transaction block is skipped — including the bather conflict query on lines 492–516. A groomer/bather can be double-booked through this path.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/api/src/routes/appointments.ts
   Line: 424-428
   
   Comment:
   **Bather-only PATCH skips conflict check entirely**
   
   `updateFields.batherStaffId !== undefined` was removed from `needsConflictCheck`. When a client sends a PATCH that only changes the bather (e.g. `{ "batherStaffId": "uuid" }`), `needsConflictCheck` is `false` and the entire conflict-check transaction block is skipped — including the bather conflict query on lines 492–516. A groomer/bather can be double-booked through this path.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.
   

5. apps/api/src/routes/appointments.ts, line 380-408 (link)

   Cascade update loop no longer checks for scheduling conflicts

   The previous implementation ran both staffId and batherStaffId conflict queries for each appointment being shifted in the series, and the 409 response path in the catch block was present to surface them. Both the per-appointment conflict queries and the 409 error handler were removed from this PR.

   A cascaded reschedule (e.g. "shift all future appointments in this series one hour earlier") can now silently move an entire series into time slots already occupied by other appointments, double-booking the groomer and/or bather across potentially dozens of appointments at once.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/api/src/routes/appointments.ts
   Line: 380-408
   
   Comment:
   **Cascade update loop no longer checks for scheduling conflicts**
   
   The previous implementation ran both `staffId` and `batherStaffId` conflict queries for each appointment being shifted in the series, and the 409 response path in the catch block was present to surface them. Both the per-appointment conflict queries and the 409 error handler were removed from this PR.
   
   A cascaded reschedule (e.g. "shift all future appointments in this series one hour earlier") can now silently move an entire series into time slots already occupied by other appointments, double-booking the groomer and/or bather across potentially dozens of appointments at once.
   
   How can I resolve this? If you propose a fix, please make it concise.
   

6. apps/api/src/routes/invoices.ts, line 173-178 (link)

   Breaking response shape change on POST /:id/tip-splits

   The endpoint previously returned { ...updatedInvoice, lineItems, tipSplits } — the full enriched invoice object. It now returns a bare array of split rows. Any frontend or integration client that destructures the old shape (e.g. result.lineItems, result.status) will silently receive undefined for every field.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/api/src/routes/invoices.ts
   Line: 173-178
   
   Comment:
   **Breaking response shape change on `POST /:id/tip-splits`**
   
   The endpoint previously returned `{ ...updatedInvoice, lineItems, tipSplits }` — the full enriched invoice object. It now returns a bare array of split rows. Any frontend or integration client that destructures the old shape (e.g. `result.lineItems`, `result.status`) will silently receive `undefined` for every field.
   
   How can I resolve this? If you propose a fix, please make it concise.
   

Prompt To Fix All With AI

This is a comment left during a code review.
Path: apps/api/src/routes/groomingLogs.ts
Line: 59-105

Comment:
**`appointmentId` silently dropped from every new grooming log**

`appointmentId` is destructured out of `rest` on line 59 but is never re-added to the `.values(...)` call (lines 99–105). Every new grooming visit log will be stored with a `null` appointment linkage, regardless of what the caller supplies. The original code passed it explicitly as `appointmentId: appointmentId ?? null`.

Fix: re-include `appointmentId` in the insert:
```ts
const [row] = await db
  .insert(groomingVisitLogs)
  .values({
    ...rest,
    appointmentId: appointmentId ?? null,
    groomedAt: groomedAt ? new Date(groomedAt) : new Date(),
  })
  .returning();
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/api/src/routes/invoices.ts
Line: 382-385

Comment:
**Refund idempotency and audit trail removed — double-charge risk**

The previous implementation:
1. Accepted an `idempotencyKey` and short-circuited on duplicates (preventing network-retry double refunds).
2. Persisted every refund record to the `refunds` table inside a transaction for audit purposes.

Both safety mechanisms were removed. Now a transient network error or client retry will call `processRefund` twice, issuing two refunds to Stripe. There is also no database record of issued refunds.

Consider restoring at minimum the idempotency guard — even a simple `refunds` table insert with an `ON CONFLICT DO NOTHING` on `stripeRefundId` provides a meaningful safety net.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/api/src/routes/invoices.ts
Line: 318-320

Comment:
**Status state-machine removed — invalid transitions now accepted**

The `ALLOWED_TRANSITIONS` guard was replaced with only a void check, so the following transitions are now silently accepted by the API:

- `paid → draft` / `paid → pending` (un-paying a settled invoice)
- `pending → draft` (already-sent invoice rolled back to draft)

The previous state machine modelled real business constraints. At a minimum, `paid` invoices should not be moveable to `draft` or `pending`, which could create incorrect financial state.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/api/src/routes/appointments.ts
Line: 424-428

Comment:
**Bather-only PATCH skips conflict check entirely**

`updateFields.batherStaffId !== undefined` was removed from `needsConflictCheck`. When a client sends a PATCH that only changes the bather (e.g. `{ "batherStaffId": "uuid" }`), `needsConflictCheck` is `false` and the entire conflict-check transaction block is skipped — including the bather conflict query on lines 492–516. A groomer/bather can be double-booked through this path.

```suggestion
    const needsConflictCheck =
      updateFields.startTime !== undefined ||
      updateFields.endTime !== undefined ||
      updateFields.staffId !== undefined ||
      updateFields.batherStaffId !== undefined;
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/api/src/routes/appointments.ts
Line: 380-408

Comment:
**Cascade update loop no longer checks for scheduling conflicts**

The previous implementation ran both `staffId` and `batherStaffId` conflict queries for each appointment being shifted in the series, and the 409 response path in the catch block was present to surface them. Both the per-appointment conflict queries and the 409 error handler were removed from this PR.

A cascaded reschedule (e.g. "shift all future appointments in this series one hour earlier") can now silently move an entire series into time slots already occupied by other appointments, double-booking the groomer and/or bather across potentially dozens of appointments at once.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/api/src/routes/invoices.ts
Line: 173-178

Comment:
**Breaking response shape change on `POST /:id/tip-splits`**

The endpoint previously returned `{ ...updatedInvoice, lineItems, tipSplits }` — the full enriched invoice object. It now returns a bare array of split rows. Any frontend or integration client that destructures the old shape (e.g. `result.lineItems`, `result.status`) will silently receive `undefined` for every field.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/api/src/routes/stripe-webhooks.ts
Line: 48-50

Comment:
**`parsed.data` unused — `invoiceId.trim()` called twice**

After the `safeParse` guard on line 48, `parsed.data` already holds the trimmed, validated UUID. Creating a second `invoiceIdTrimmed` variable by calling `.trim()` again is redundant. Use `parsed.data` directly:

```suggestion
        const parsed = z.string().uuid().safeParse(invoiceId.trim());
        if (!parsed.success) continue;
        const invoiceIdTrimmed = parsed.data;
```

The same pattern appears in the `payment_intent.payment_failed` handler (lines 76–78).

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(stripe-webhooks): validate invoice I..." | Re-trigger Greptile}

<h3>Greptile Summary</h3> This PR adds the five targeted input validations described in the PR description (invoice query params, booking future-time check, recurrence 1-year cap, 8-hour service duration cap, and Stripe UUID guard), all of which are correct. However, the diff includes a much wider set of behavioral changes that are not mentioned in the description, and several of them introduce regressions: - **P0 — data loss (`groomingLogs.ts`)**: `appointmentId` is destructured out of the request body but never included in the DB insert. Every grooming log created after this lands will have a `null` appointment link regardless of what the caller provides. - **P0 — double-refund risk (`invoices.ts`)**: The refund endpoint lost its idempotency-key guard and transaction-backed `refunds` table insert. A network retry or duplicate request will call `processRefund` twice, resulting in two Stripe refunds. - **P1 — invalid invoice transitions (`invoices.ts`)**: The `ALLOWED_TRANSITIONS` state machine was replaced with a void-only check. Paid invoices can now be moved back to `draft` or `pending` through the API. - **P1 — bather-only PATCH skips conflict check (`appointments.ts`)**: `updateFields.batherStaffId` was removed from `needsConflictCheck`, so a PATCH that only changes the bather staff member bypasses the scheduling conflict block entirely. - **P1 — cascade update has no conflict checking (`appointments.ts`)**: The per-appointment staffId/batherStaffId conflict queries were removed from the cascade update loop, allowing a cascaded reschedule to silently double-book a groomer across an entire series. - **P1 — breaking API shape change (`invoices.ts`)**: `POST /:id/tip-splits` now returns a bare array instead of the enriched invoice object, which will break any client expecting the previous shape. <h3>Confidence Score: 1/5</h3> Not safe to merge — two P0 bugs (data loss on grooming logs, double-refund risk) and three P1 regressions need to be resolved first. The five advertised validations are all correct, but the PR bundles a wide set of undescribed behavioral changes. Two are critical: `appointmentId` is permanently dropped from every new grooming log (silent data corruption), and the refund endpoint lost its idempotency guard (financial double-charge risk on any retry). Three further P1 bugs — invalid invoice status transitions, bather-only PATCH bypassing conflict checks, and cascade updates losing all conflict detection — add scheduling reliability concerns. The PR needs targeted fixes before landing. `apps/api/src/routes/groomingLogs.ts` (appointmentId drop), `apps/api/src/routes/invoices.ts` (refund safety, status transitions, tip-splits response), `apps/api/src/routes/appointments.ts` (cascade and bather conflict checks) <details><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | apps/api/src/routes/groomingLogs.ts | P0 data-loss bug: `appointmentId` is destructured out of the body but never re-added to the DB insert, so every new grooming log is silently stored with a null appointment link. | | apps/api/src/routes/invoices.ts | Three regressions: (1) refund idempotency key and audit-trail transaction removed (double-refund risk); (2) invoice status state-machine replaced with void-only guard (paid→draft now allowed); (3) tip-splits response shape changed from enriched invoice object to bare array (breaking API change). | | apps/api/src/routes/appointments.ts | Two conflict-check regressions: cascade update loop lost all staffId/batherStaffId conflict queries; batherStaffId-only PATCH bypasses the conflict check block entirely. The new recurrence 1-year cap (frequencyWeeks × count ≤ 52) is correct. | | apps/api/src/routes/stripe-webhooks.ts | UUID guard added correctly; minor: `parsed.data` unused, `invoiceId.trim()` called twice instead. | | apps/api/src/routes/services.ts | Clean addition of `.max(480)` on `durationMinutes`; no issues. | | apps/api/src/routes/book.ts | Future-time Zod refinement on `startTime` is correct. Confirm/cancel token handlers simplified from atomic WHERE clause to ID-based update with pre-checks; idempotent in practice but introduces a TOCTOU window. | | apps/api/src/routes/appointmentGroups.ts | Refactoring of groomer access checks into `hasAccess` intermediate variable; logic preserved. One removed `if (!updated) return 404` after an already-checked existence query — benign. | </details> </details> <h3>Sequence Diagram</h3> ```mermaid sequenceDiagram participant Client participant API participant DB participant Stripe Note over Client,Stripe: Refund flow — BEFORE this PR (safe) Client->>API: POST /invoices/:id/refund {amountCents, idempotencyKey} API->>DB: BEGIN TRANSACTION DB->>DB: SELECT refunds WHERE idempotencyKey = ? alt Duplicate request DB-->>API: existing refund found API-->>Client: 200 {refundId} (idempotent) else First request API->>Stripe: processRefund() Stripe-->>API: {refundId} API->>DB: INSERT refunds (audit trail) DB-->>API: OK API->>DB: COMMIT API-->>Client: 200 {refundId} end Note over Client,Stripe: Refund flow — AFTER this PR (risk) Client->>API: POST /invoices/:id/refund {amountCents} API->>Stripe: processRefund() Stripe-->>API: {refundId} API-->>Client: 200 {refundId} Note over Client,Stripe: Retry → second processRefund() call → double charge ``` <!-- greptile_failed_comments --> <details open><summary><h3>Comments Outside Diff (6)</h3></summary> 1. `apps/api/src/routes/groomingLogs.ts`, line 59-105 ([link](https://github.com/groombook/groombook/blob/203b600713fc7ecdaf02c88d1a51d562f651070a/apps/api/src/routes/groomingLogs.ts#L59-L105)) <a href="#"><img alt="P0" src="https://greptile-static-assets.s3.amazonaws.com/badges/p0.svg?v=7" align="top"></a> **`appointmentId` silently dropped from every new grooming log** `appointmentId` is destructured out of `rest` on line 59 but is never re-added to the `.values(...)` call (lines 99–105). Every new grooming visit log will be stored with a `null` appointment linkage, regardless of what the caller supplies. The original code passed it explicitly as `appointmentId: appointmentId ?? null`. Fix: re-include `appointmentId` in the insert: ```ts const [row] = await db .insert(groomingVisitLogs) .values({ ...rest, appointmentId: appointmentId ?? null, groomedAt: groomedAt ? new Date(groomedAt) : new Date(), }) .returning(); ``` <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/routes/groomingLogs.ts Line: 59-105 Comment: **`appointmentId` silently dropped from every new grooming log** `appointmentId` is destructured out of `rest` on line 59 but is never re-added to the `.values(...)` call (lines 99–105). Every new grooming visit log will be stored with a `null` appointment linkage, regardless of what the caller supplies. The original code passed it explicitly as `appointmentId: appointmentId ?? null`. Fix: re-include `appointmentId` in the insert: ```ts const [row] = await db .insert(groomingVisitLogs) .values({ ...rest, appointmentId: appointmentId ?? null, groomedAt: groomedAt ? new Date(groomedAt) : new Date(), }) .returning(); ``` How can I resolve this? If you propose a fix, please make it concise. ````` </details> 2. `apps/api/src/routes/invoices.ts`, line 382-385 ([link](https://github.com/groombook/groombook/blob/203b600713fc7ecdaf02c88d1a51d562f651070a/apps/api/src/routes/invoices.ts#L382-L385)) <a href="#"><img alt="P0" src="https://greptile-static-assets.s3.amazonaws.com/badges/p0.svg?v=7" align="top"></a> **Refund idempotency and audit trail removed — double-charge risk** The previous implementation: 1. Accepted an `idempotencyKey` and short-circuited on duplicates (preventing network-retry double refunds). 2. Persisted every refund record to the `refunds` table inside a transaction for audit purposes. Both safety mechanisms were removed. Now a transient network error or client retry will call `processRefund` twice, issuing two refunds to Stripe. There is also no database record of issued refunds. Consider restoring at minimum the idempotency guard — even a simple `refunds` table insert with an `ON CONFLICT DO NOTHING` on `stripeRefundId` provides a meaningful safety net. <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/routes/invoices.ts Line: 382-385 Comment: **Refund idempotency and audit trail removed — double-charge risk** The previous implementation: 1. Accepted an `idempotencyKey` and short-circuited on duplicates (preventing network-retry double refunds). 2. Persisted every refund record to the `refunds` table inside a transaction for audit purposes. Both safety mechanisms were removed. Now a transient network error or client retry will call `processRefund` twice, issuing two refunds to Stripe. There is also no database record of issued refunds. Consider restoring at minimum the idempotency guard — even a simple `refunds` table insert with an `ON CONFLICT DO NOTHING` on `stripeRefundId` provides a meaningful safety net. How can I resolve this? If you propose a fix, please make it concise. ````` </details> 3. `apps/api/src/routes/invoices.ts`, line 318-320 ([link](https://github.com/groombook/groombook/blob/203b600713fc7ecdaf02c88d1a51d562f651070a/apps/api/src/routes/invoices.ts#L318-L320)) <a href="#"><img alt="P1" src="https://greptile-static-assets.s3.amazonaws.com/badges/p1.svg?v=7" align="top"></a> **Status state-machine removed — invalid transitions now accepted** The `ALLOWED_TRANSITIONS` guard was replaced with only a void check, so the following transitions are now silently accepted by the API: - `paid → draft` / `paid → pending` (un-paying a settled invoice) - `pending → draft` (already-sent invoice rolled back to draft) The previous state machine modelled real business constraints. At a minimum, `paid` invoices should not be moveable to `draft` or `pending`, which could create incorrect financial state. <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/routes/invoices.ts Line: 318-320 Comment: **Status state-machine removed — invalid transitions now accepted** The `ALLOWED_TRANSITIONS` guard was replaced with only a void check, so the following transitions are now silently accepted by the API: - `paid → draft` / `paid → pending` (un-paying a settled invoice) - `pending → draft` (already-sent invoice rolled back to draft) The previous state machine modelled real business constraints. At a minimum, `paid` invoices should not be moveable to `draft` or `pending`, which could create incorrect financial state. How can I resolve this? If you propose a fix, please make it concise. ````` </details> 4. `apps/api/src/routes/appointments.ts`, line 424-428 ([link](https://github.com/groombook/groombook/blob/203b600713fc7ecdaf02c88d1a51d562f651070a/apps/api/src/routes/appointments.ts#L424-L428)) <a href="#"><img alt="P1" src="https://greptile-static-assets.s3.amazonaws.com/badges/p1.svg?v=7" align="top"></a> **Bather-only PATCH skips conflict check entirely** `updateFields.batherStaffId !== undefined` was removed from `needsConflictCheck`. When a client sends a PATCH that only changes the bather (e.g. `{ "batherStaffId": "uuid" }`), `needsConflictCheck` is `false` and the entire conflict-check transaction block is skipped — including the bather conflict query on lines 492–516. A groomer/bather can be double-booked through this path. <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/routes/appointments.ts Line: 424-428 Comment: **Bather-only PATCH skips conflict check entirely** `updateFields.batherStaffId !== undefined` was removed from `needsConflictCheck`. When a client sends a PATCH that only changes the bather (e.g. `{ "batherStaffId": "uuid" }`), `needsConflictCheck` is `false` and the entire conflict-check transaction block is skipped — including the bather conflict query on lines 492–516. A groomer/bather can be double-booked through this path. How can I resolve this? If you propose a fix, please make it concise. ````` </details> 5. `apps/api/src/routes/appointments.ts`, line 380-408 ([link](https://github.com/groombook/groombook/blob/203b600713fc7ecdaf02c88d1a51d562f651070a/apps/api/src/routes/appointments.ts#L380-L408)) <a href="#"><img alt="P1" src="https://greptile-static-assets.s3.amazonaws.com/badges/p1.svg?v=7" align="top"></a> **Cascade update loop no longer checks for scheduling conflicts** The previous implementation ran both `staffId` and `batherStaffId` conflict queries for each appointment being shifted in the series, and the 409 response path in the catch block was present to surface them. Both the per-appointment conflict queries and the 409 error handler were removed from this PR. A cascaded reschedule (e.g. "shift all future appointments in this series one hour earlier") can now silently move an entire series into time slots already occupied by other appointments, double-booking the groomer and/or bather across potentially dozens of appointments at once. <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/routes/appointments.ts Line: 380-408 Comment: **Cascade update loop no longer checks for scheduling conflicts** The previous implementation ran both `staffId` and `batherStaffId` conflict queries for each appointment being shifted in the series, and the 409 response path in the catch block was present to surface them. Both the per-appointment conflict queries and the 409 error handler were removed from this PR. A cascaded reschedule (e.g. "shift all future appointments in this series one hour earlier") can now silently move an entire series into time slots already occupied by other appointments, double-booking the groomer and/or bather across potentially dozens of appointments at once. How can I resolve this? If you propose a fix, please make it concise. ````` </details> 6. `apps/api/src/routes/invoices.ts`, line 173-178 ([link](https://github.com/groombook/groombook/blob/203b600713fc7ecdaf02c88d1a51d562f651070a/apps/api/src/routes/invoices.ts#L173-L178)) <a href="#"><img alt="P1" src="https://greptile-static-assets.s3.amazonaws.com/badges/p1.svg?v=7" align="top"></a> **Breaking response shape change on `POST /:id/tip-splits`** The endpoint previously returned `{ ...updatedInvoice, lineItems, tipSplits }` — the full enriched invoice object. It now returns a bare array of split rows. Any frontend or integration client that destructures the old shape (e.g. `result.lineItems`, `result.status`) will silently receive `undefined` for every field. <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/routes/invoices.ts Line: 173-178 Comment: **Breaking response shape change on `POST /:id/tip-splits`** The endpoint previously returned `{ ...updatedInvoice, lineItems, tipSplits }` — the full enriched invoice object. It now returns a bare array of split rows. Any frontend or integration client that destructures the old shape (e.g. `result.lineItems`, `result.status`) will silently receive `undefined` for every field. How can I resolve this? If you propose a fix, please make it concise. ````` </details> </details> <!-- /greptile_failed_comments --> <details><summary>Prompt To Fix All With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/routes/groomingLogs.ts Line: 59-105 Comment: **`appointmentId` silently dropped from every new grooming log** `appointmentId` is destructured out of `rest` on line 59 but is never re-added to the `.values(...)` call (lines 99–105). Every new grooming visit log will be stored with a `null` appointment linkage, regardless of what the caller supplies. The original code passed it explicitly as `appointmentId: appointmentId ?? null`. Fix: re-include `appointmentId` in the insert: ```ts const [row] = await db .insert(groomingVisitLogs) .values({ ...rest, appointmentId: appointmentId ?? null, groomedAt: groomedAt ? new Date(groomedAt) : new Date(), }) .returning(); ``` How can I resolve this? If you propose a fix, please make it concise. --- This is a comment left during a code review. Path: apps/api/src/routes/invoices.ts Line: 382-385 Comment: **Refund idempotency and audit trail removed — double-charge risk** The previous implementation: 1. Accepted an `idempotencyKey` and short-circuited on duplicates (preventing network-retry double refunds). 2. Persisted every refund record to the `refunds` table inside a transaction for audit purposes. Both safety mechanisms were removed. Now a transient network error or client retry will call `processRefund` twice, issuing two refunds to Stripe. There is also no database record of issued refunds. Consider restoring at minimum the idempotency guard — even a simple `refunds` table insert with an `ON CONFLICT DO NOTHING` on `stripeRefundId` provides a meaningful safety net. How can I resolve this? If you propose a fix, please make it concise. --- This is a comment left during a code review. Path: apps/api/src/routes/invoices.ts Line: 318-320 Comment: **Status state-machine removed — invalid transitions now accepted** The `ALLOWED_TRANSITIONS` guard was replaced with only a void check, so the following transitions are now silently accepted by the API: - `paid → draft` / `paid → pending` (un-paying a settled invoice) - `pending → draft` (already-sent invoice rolled back to draft) The previous state machine modelled real business constraints. At a minimum, `paid` invoices should not be moveable to `draft` or `pending`, which could create incorrect financial state. How can I resolve this? If you propose a fix, please make it concise. --- This is a comment left during a code review. Path: apps/api/src/routes/appointments.ts Line: 424-428 Comment: **Bather-only PATCH skips conflict check entirely** `updateFields.batherStaffId !== undefined` was removed from `needsConflictCheck`. When a client sends a PATCH that only changes the bather (e.g. `{ "batherStaffId": "uuid" }`), `needsConflictCheck` is `false` and the entire conflict-check transaction block is skipped — including the bather conflict query on lines 492–516. A groomer/bather can be double-booked through this path. ```suggestion const needsConflictCheck = updateFields.startTime !== undefined || updateFields.endTime !== undefined || updateFields.staffId !== undefined || updateFields.batherStaffId !== undefined; ``` How can I resolve this? If you propose a fix, please make it concise. --- This is a comment left during a code review. Path: apps/api/src/routes/appointments.ts Line: 380-408 Comment: **Cascade update loop no longer checks for scheduling conflicts** The previous implementation ran both `staffId` and `batherStaffId` conflict queries for each appointment being shifted in the series, and the 409 response path in the catch block was present to surface them. Both the per-appointment conflict queries and the 409 error handler were removed from this PR. A cascaded reschedule (e.g. "shift all future appointments in this series one hour earlier") can now silently move an entire series into time slots already occupied by other appointments, double-booking the groomer and/or bather across potentially dozens of appointments at once. How can I resolve this? If you propose a fix, please make it concise. --- This is a comment left during a code review. Path: apps/api/src/routes/invoices.ts Line: 173-178 Comment: **Breaking response shape change on `POST /:id/tip-splits`** The endpoint previously returned `{ ...updatedInvoice, lineItems, tipSplits }` — the full enriched invoice object. It now returns a bare array of split rows. Any frontend or integration client that destructures the old shape (e.g. `result.lineItems`, `result.status`) will silently receive `undefined` for every field. How can I resolve this? If you propose a fix, please make it concise. --- This is a comment left during a code review. Path: apps/api/src/routes/stripe-webhooks.ts Line: 48-50 Comment: **`parsed.data` unused — `invoiceId.trim()` called twice** After the `safeParse` guard on line 48, `parsed.data` already holds the trimmed, validated UUID. Creating a second `invoiceIdTrimmed` variable by calling `.trim()` again is redundant. Use `parsed.data` directly: ```suggestion const parsed = z.string().uuid().safeParse(invoiceId.trim()); if (!parsed.success) continue; const invoiceIdTrimmed = parsed.data; ``` The same pattern appears in the `payment_intent.payment_failed` handler (lines 76–78). How can I resolve this? If you propose a fix, please make it concise. ````` </details> _{Reviews (1): Last reviewed commit: ["fix(stripe-webhooks): validate invoice I..."](https://github.com/groombook/groombook/commit/203b600713fc7ecdaf02c88d1a51d562f651070a) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=28624629)}

greptile-apps[bot] (Migrated from github.com) reviewed 2026-04-16 12:31:36 +00:00

apps/api/src/routes/stripe-webhooks.ts

greptile-apps[bot] (Migrated from github.com) commented 2026-04-16 12:31:34 +00:00

parsed.data unused — invoiceId.trim() called twice

After the safeParse guard on line 48, parsed.data already holds the trimmed, validated UUID. Creating a second invoiceIdTrimmed variable by calling .trim() again is redundant. Use parsed.data directly:

        const parsed = z.string().uuid().safeParse(invoiceId.trim());
        if (!parsed.success) continue;
        const invoiceIdTrimmed = parsed.data;

The same pattern appears in the payment_intent.payment_failed handler (lines 76–78).

Prompt To Fix With AI

This is a comment left during a code review.
Path: apps/api/src/routes/stripe-webhooks.ts
Line: 48-50

Comment:
**`parsed.data` unused — `invoiceId.trim()` called twice**

After the `safeParse` guard on line 48, `parsed.data` already holds the trimmed, validated UUID. Creating a second `invoiceIdTrimmed` variable by calling `.trim()` again is redundant. Use `parsed.data` directly:

```suggestion
        const parsed = z.string().uuid().safeParse(invoiceId.trim());
        if (!parsed.success) continue;
        const invoiceIdTrimmed = parsed.data;
```

The same pattern appears in the `payment_intent.payment_failed` handler (lines 76–78).

How can I resolve this? If you propose a fix, please make it concise.

<a href="#"><img alt="P2" src="https://greptile-static-assets.s3.amazonaws.com/badges/p2.svg?v=7" align="top"></a> **`parsed.data` unused — `invoiceId.trim()` called twice** After the `safeParse` guard on line 48, `parsed.data` already holds the trimmed, validated UUID. Creating a second `invoiceIdTrimmed` variable by calling `.trim()` again is redundant. Use `parsed.data` directly: ```suggestion const parsed = z.string().uuid().safeParse(invoiceId.trim()); if (!parsed.success) continue; const invoiceIdTrimmed = parsed.data; ``` The same pattern appears in the `payment_intent.payment_failed` handler (lines 76–78). <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/routes/stripe-webhooks.ts Line: 48-50 Comment: **`parsed.data` unused — `invoiceId.trim()` called twice** After the `safeParse` guard on line 48, `parsed.data` already holds the trimmed, validated UUID. Creating a second `invoiceIdTrimmed` variable by calling `.trim()` again is redundant. Use `parsed.data` directly: ```suggestion const parsed = z.string().uuid().safeParse(invoiceId.trim()); if (!parsed.success) continue; const invoiceIdTrimmed = parsed.data; ``` The same pattern appears in the `payment_intent.payment_failed` handler (lines 76–78). How can I resolve this? If you propose a fix, please make it concise. ````` </details>

the-dogfather-cto[bot] (Migrated from github.com) requested changes 2026-04-16 12:32:18 +00:00

the-dogfather-cto[bot] (Migrated from github.com) left a comment

CTO Review — Changes Requested

Validation fixes: APPROVED ✅

All 5 input validation fixes are correctly implemented:

#	Fix	Assessment
1	Invoice query params (listInvoicesQuerySchema + zValidator)	Correct — proper UUID, enum, coerce types
2	Future-time booking (startTime.refine())	Correct
3	Recurrence cap (frequencyWeeks * count <= 52)	Correct
4	Service duration (.max(480), inherits via .partial())	Correct
5	Stripe webhook UUID validation (safeParse guard)	Correct

Merge conflict: BLOCKING ❌

This branch contains an out-of-scope commit from GRO-623/GRO-635:

5e24678 feat(GRO-635): implement groomer data isolation in appointmentGroups,...

That work was already merged to dev via PR #285. The duplicate commit causes merge conflicts in appointmentGroups.ts, groomingLogs.ts, and appointments.ts.

Required fix

Rebase fix/gro-624-input-validation onto dev and drop commit 5e24678. The 5 validation commits (369c2ce through 203b600) should rebase cleanly once the GRO-635 commit is removed.

git fetch origin dev
git rebase -i origin/dev
# Drop the 5e24678 (GRO-635) commit, keep the 5 validation commits
git push --force-with-lease

After force-push, CI will re-run and the merge conflict should be resolved.

cc @cpfarhood

## CTO Review — Changes Requested ### Validation fixes: APPROVED ✅ All 5 input validation fixes are correctly implemented: | # | Fix | Assessment | |---|-----|-----------| | 1 | Invoice query params (`listInvoicesQuerySchema` + `zValidator`) | Correct — proper UUID, enum, coerce types | | 2 | Future-time booking (`startTime.refine()`) | Correct | | 3 | Recurrence cap (`frequencyWeeks * count <= 52`) | Correct | | 4 | Service duration (`.max(480)`, inherits via `.partial()`) | Correct | | 5 | Stripe webhook UUID validation (`safeParse` guard) | Correct | ### Merge conflict: BLOCKING ❌ This branch contains an out-of-scope commit from GRO-623/GRO-635: ``` 5e24678 feat(GRO-635): implement groomer data isolation in appointmentGroups,... ``` That work was already merged to `dev` via PR #285. The duplicate commit causes merge conflicts in `appointmentGroups.ts`, `groomingLogs.ts`, and `appointments.ts`. ### Required fix Rebase `fix/gro-624-input-validation` onto `dev` and **drop** commit `5e24678`. The 5 validation commits (`369c2ce` through `203b600`) should rebase cleanly once the GRO-635 commit is removed. ```bash git fetch origin dev git rebase -i origin/dev # Drop the 5e24678 (GRO-635) commit, keep the 5 validation commits git push --force-with-lease ``` After force-push, CI will re-run and the merge conflict should be resolved. cc @cpfarhood

This repo is archived. You cannot comment on pull requests.

Reviewers

No Reviewers

lint-roller-qa[bot]

greptile-apps[bot]

the-dogfather-cto[bot]

Labels

Clear labels

bug

Something isn't working

documentation

Improvements or additions to documentation

duplicate

This issue or pull request already exists

enhancement

New feature or request

feature

New feature

good first issue

Good for newcomers

help wanted

Extra attention is needed

invalid

This doesn't seem right

question

Further information is requested

wontfix

This will not be worked on

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

ai-review (AI Review) gb_barkley (Barkley Trimsworth) cpfarhood (Chris Farhood) ci (Continuous Integration [bot]) gb_flea (Flea Flicker) flux (Flux CD) admin (Gitea Admin) gb_lint (Lint Roller) renovate (Mend Renovate) gb_pawla (Pawla Abdul) gb_scrubs (Scrubs McBarkley) gb_shedward (Shedward Scissorhands) gb_dogfather (The Dogfather)

No Assignees

1 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: groombook/app#305