# Grill review: Sales Order deposit missing from End Shift

Plan grilled: `deposit-sales-order-end-shift-plan.html` (HTML only, no `plan.md`).
Against: `db-postgresql/tenants.schema/functions_pg.sql`, `migrations/tenant/versions/20260921_0200_rename_workflow_status_functions.py`, `apps/v2-pos-client/client/lib`, `apps/v2-sellable-api/src`.
Date: 2026-10-23. Round 1 of 2. Agents: DATABASE, API+CLIENT, COVERAGE verifiers + 1 red-team critic. The lead re-checked the two central facts (marked *).

## IMPLEMENTATION READINESS: NOT READY

## Agreed findings (all 4 agents)

### CRITICAL
1. **Root cause is wrong on the server.** `payment_amounts` (CLOSE_FLOAT) in `fn_api_dayshift_get_xreport_shift_summary` filters only `is_voided`, `tender_category NOT IN (STORE CREDIT, GIFT CARD)` and `tender_code NOT IN (EX_FORFEITED, EXCHANGE_EXCESS)`. It has **no doc_type or status filter** (`functions_pg.sql:1911-1922`*; same in the 20260921_0200 migration, lines 216-226). A DRAFT Sales Order cash deposit in the same shift is already counted. The filter quoted in plan §2B belongs to the XYZ sales-summary CTEs (`:2297-2298`) and `vw_dayshift_getdayallshifts` (`views_pg.sql:432-477`), not the drawer sum.
2. **Step 1 is a regression.** It adds a status filter where none exists. It would drop `COMPLETED_DISPATCHED`, VOIDED credit invoices and cancelled SOs with refund rows from expected cash. Its snippet also deletes the `tender_code NOT IN (EX_FORFEITED, EXCHANGE_EXCESS)` exclusion (migrations `20260729_0300`, `20260730_0100`). It uses a `{SCHEMA}` placeholder; the real function hard-codes `sellable_tenant`.
3. **Step 1 would break shift close.** Close calls `compare_dayshift_summary` (`dayshift_service.py:1643`), which recomputes with a different function, `fn_api_get_shift_summary` (`functions_pg.sql:2708`, no status filter). Changing only the X-report function makes the two disagree. Close is rejected with "Expected amount ... not match". Not mentioned in the plan. (API verifier only; not independently re-checked.)
4. **Step 2 edits code End Shift never runs.** End Shift expected cash is server-only (`shift_close_controller.dart:136-176`; on failure it shows an error, there is no local fallback). `getSalesReportByCash` has one caller, the dashboard (`register_information_controller.dart:109`*). `getSalesReportByTenderType` has no callers*. The "offline client path" does not exist. The proposed `localAmt > 0` filter would also drop negative change and refund rows and overstate dashboard cash.
5. **The "tag the payment with Shift ID" fix is never built, and the real defect is probably here.** `ft_sales_payment` has no dayshift column (`tables_pg.sql:3201-3265`). Every drawer query uses the **header's** `dayshift_id`. A top-up deposit taken in shift 2 on an SO made in shift 1 goes through `list_deposit_dialog_controller.dart:1373-1393`. That code keeps the original header shift. So the cash lands in shift 1 (maybe already closed) and is missing from shift 2. This fits the symptom better than the plan's theory. Same for deposit refunds (negative rows on the SO, `:797-841`) and voids (`sale_pre_special_order_utils.dart:42-90`).
6. **Nobody reproduced the bug.** The same-shift scenario (TC-01) already counts the deposit by the code as written. The plan's "current result" columns for TC-02 and TC-04 cannot be reproduced.

### HIGH
7. **Deposit line already exists.** `XYZ_SALES_DEPOSIT` ("Sales Deposit") is computed for DRAFT SOs (`functions_pg.sql:2372-2381`, `:6593`, `views_pg.sql:647-655`). The client constant is `day_shift_constants.dart:37` and is never rendered. The plan invents a new `DEPOSIT` row that no code writes or reads. That CTE keys on `status='DRAFT'`, so after the SO is finalized the earlier shift's reprint loses its deposit line. Not mentioned.
8. **§6 SQL fixture cannot run.** `dayshift_id`, `sales_id`, `sales_payment_id`, `dayshift_payment_id` are `GENERATED ALWAYS AS IDENTITY`. `ft_dayshift.dayshift_type` is NOT NULL and omitted. `tender_code` is NULL, so `NULL NOT IN (...)` drops the row in the real CTE. There are no FKs and the keys are composite and partitioned, so the "bigint (FK)" labels are wrong. `company_id` is missing on the header.
9. **Missing essentials.** No Alembic revision name or down/rollback. No update to canonical `functions_pg.sql`. No rollout order. No stance on already-closed shifts (`ft_dayshift_payments` is written at close). No automated tests, although `apps/v2-sellable-bo/backend/tests/unit/repositories/test_business_day_summary_footing_597.py` exists. Step 3 UI/print lines have no file, and need i18n keys in 4 locales (client rules I1-I3).
10. **Coverage gaps.** No case for foreign-currency deposits (`forexChgAmt`), several deposits on one SO across shifts, refund or void of a deposit, SO edited after deposit, or deposits that were not yet synced at End Shift (offline path).
11. **"Status: Ready for Implementation" is not honest** given 1-6. No `plan.md` exists next to the `.html`.

### MEDIUM
- Other consumers not mentioned: `fn_api_set_dayshift_payment` (writes close rows), `vw_dayshift_getdayallshifts`, BO `business_day_repository.py` / `business_day_service.py` / `sales_doc_semantics.py`, `sale_session_controller.dart:1037,1471`.
- X-Report sample receipts use "$" for a MYR store, invent print sections that `PrintReportTemplate` does not produce, and the header says "SHORT -$50" while the body says +50 over.
- Option B claim "PREPAY_INV already allowed by shift function" is true only for the XYZ sales sections, not CLOSE_FLOAT.
- Store credit and gift card deposits: excluded consistently, but not stated or tested.
- Competitor claims in §3 (Shopify `register_shift_id` SQL, Toast/Square wording) are uncited.

### LOW
- Line references are off by one or two (PREPAY_INV `:2832`, DAO method `:114`).
- Emoji in headings (project rule: no emoji in docs).

## Conflicts and resolution
No agent disagreed. One difference of emphasis: DATABASE rated cross-shift attribution HIGH, COVERAGE rated it CRITICAL. Decision: **CRITICAL**. It is the only mechanism the code supports for the reported symptom.

## Refutation pass
The lead re-read `functions_pg.sql:1905-1930` and grepped both DAO methods. Both central claims hold. No findings were dropped. A separate refute agent was judged unneeded: four independent agents agree and the two key facts were checked first-hand.

## Unverified (risk, not pass)
- Whether the **deployed** dev/prod function body matches `functions_pg.sql` (the `wf_status_vocab_*` migrations may not be applied there). No live DB was queried.
- How the sales upload sets `dayshift_id` on later updates to an SO header (`sales_query.py:1590`, `:1636-1643`).
- Whether a failed online refresh leaves deposit payments only local (`sale_utils.dart:1222-1236`).
- The real cause of the reported +$50 gap.

## Already solid
- Client code at `new_sale_order_controller.dart:2817-2828` (SO stays DRAFT) and the commented PREPAY_INV block (`:2830-2852`).
- Route chain `getDayShiftPaymentSummary` -> `dayshift_router.py:308` -> `dayshift_query.py:1070`.
- Tables and columns named in §6 exist with the stated types, except the invented ones.
- Finalize does not copy deposit payment rows onto the invoice (`new_sale_order_controller.dart:2920-2944`, `sale_calculation_utils.dart:483-487`), so shift 2 does not double count.
- A SO cannot be voided while net deposits are above zero (`sale_pre_special_order_utils.dart:23-29`).
- Safe-drop / cash-in-out checks already include same-shift deposits.

## Required adjustments (patch list, plan not rewritten)
| Section | Change |
|---|---|
| Hero, §1, §2B | Replace root cause. Say CLOSE_FLOAT has no status filter. Mark the +$50 cause UNPROVEN until reproduced. |
| §2C, §7 Step 2 | Delete, or scope to the dashboard panel only and drop `localAmt > 0`. |
| §7 Step 1 | Delete. Do not add a status filter. |
| New step 0 | Reproduce on dev: query the SO's `ft_sales_header.dayshift_id` and its `ft_sales_payment` rows. Record raw `getDayShiftPaymentSummary` output and the deployed function body. |
| New design decision | Payment-level shift key (new column on `ft_sales_payment` + client column + sync + backfill) vs. other attribution. Decide before any code. |
| §7 Step 3, §5 | Reuse `XYZ_SALES_DEPOSIT`. Name files, i18n keys, real receipt rows. Use MYR. |
| §6 | Mark fixture illustrative, or make it valid and run it. |
| §8 | Fix "current result" for TC-02 and TC-04. Add: top-up in shift 2, refund and void of a deposit, foreign currency, unsynced SO, store credit / gift card deposit, shift close with `compare_dayshift_summary`. |
| New section | Alembic revision + down; update `functions_pg.sql`; change all of `fn_api_get_shift_summary`, `fn_api_set_dayshift_payment`, the xreport function and `vw_dayshift_getdayallshifts` together; rollout order; closed-shift stance; tests. |
| Folder | Add `plan.md`, regenerate `plan.html`, add to the docs index. |

## Path to ready
1. Reproduce the +$50 on dev and record the real data. Decide the true cause.
2. Decide shift attribution (header vs. payment-level key).
3. Rewrite the plan on the real cause. Remove Steps 1 and 2.
4. Write `plan.md` + regenerate `plan.html`. Add migration, rollout, tests, i18n.
5. Re-grill the DATABASE and COVERAGE dimensions (round 2 of 2).