diff --git a/.semgrep/README.md b/.semgrep/README.md index 4a0ccf12..2e40c5f4 100644 --- a/.semgrep/README.md +++ b/.semgrep/README.md @@ -13,7 +13,7 @@ text-scanning guardrail tests — chained call expressions split across lines. |---|---|---| | Shape | one legitimate owner file | many legitimate owners | | Owner named via | `paths.exclude` | n/a | -| Current findings | **0** | hundreds (see `baseline-counts.json`) | +| Current findings | **0** | 92 (see `baseline-counts.json`) | | CI gate | `--error`, whole tree | `--baseline-commit` (new findings only) + a per-rule count that may only go down | **Adding a rule:** if the capability has one owner and you can get the count to @@ -37,6 +37,28 @@ third, `fieldstay-supabase-unbounded-select-global-table` (tier 2c of the severity ladder below), was promoted on 2026-08-07 — see that section for its history. +**The Supabase error-handling family was promoted on 2026-08-11**: +`fieldstay-supabase-discarded-result`, `fieldstay-supabase-read-without-error` +and `fieldstay-supabase-read-without-error-fan-in`, all three at `ERROR` with +no `paths.exclude`. Nothing was fixed in that change — the burn-downs that took +them from 159 + 14 live sites to 0 are the 2026-08-07 and 2026-08-08 entries in +`baseline-counts.json`. It collected the gate upgrade those burn-downs had +earned and never taken, which matters because a rule left at 0 in `ratchet.yml` +gates only on `--baseline-commit`: a finding invisible in the diff view — a +file moved, a branch cut before the burn-down, a rewrite semgrep attributes to +neither side — still passes. `--error` across the whole tree has no such hole. + +These three are the clearest case of *handling, not ownership*: no file +legitimately owns "discard a PostgREST error", so there is no owner to name in +a `paths.exclude`, and every exemption is already expressed as the handling +constructs themselves (binding the result, destructuring `error`, going through +`lib/supabase/unwrap.ts`). Fire-checked before promoting, same protocol as tiers +2 and 2c: a scratch fixture under `lib/` carried one deliberate violation per +rule **plus a correct control for each**, and semgrep reported exactly the three +violations and none of the three controls — the controls are the half that +matters, since a rule that fires on everything also "fires". Then reverted, and +the whole tree re-run at `--error` to confirm exit 0. + ## Severity inside the ratchet family A ratchet rule whose majority is permitted-by-policy is one people learn to diff --git a/.semgrep/baseline-counts.json b/.semgrep/baseline-counts.json index a7b11ec9..a85bf3f3 100644 --- a/.semgrep/baseline-counts.json +++ b/.semgrep/baseline-counts.json @@ -5,7 +5,7 @@ "fieldstay-supabase-unbounded-select rule (284) into the severity ladder below,", "and after fieldstay-role-filtered-membership-read (3 -> 0) and", "fieldstay-untimed-external-fetch (1 -> 0) were fixed and PROMOTED to", - "chokepoints.yml — which is why they no longer appear here.", + "chokepoints.yml \u2014 which is why they no longer appear here.", "scripts/check-semgrep-ratchet.mjs re-runs the ratchet rules and FAILS if any", "count is higher than the number here. Numbers may only go DOWN: fix sites,", "then run `node scripts/check-semgrep-ratchet.mjs --update` to lock the", @@ -30,14 +30,14 @@ "PROMOTED 2026-08-02: -cross-tenant reached 0 (see above) and moved to", "chokepoints.yml, where it gates at --error across the whole tree instead of", "only on findings new vs. the PR base. Its key is deleted here in the same", - "change, per the promotion rule in .semgrep/README.md — a rule cannot be in", + "change, per the promotion rule in .semgrep/README.md \u2014 a rule cannot be in", "both files, and check-semgrep-ratchet.mjs fails on a baseline entry whose", "rule ratchet.yml no longer declares.", "", "BURNED DOWN 2026-08-07: -global-table 5 -> 0. Bounded all 5 sites with an", "explicit .limit() (or fetchAllRows for platform-inventory-template-", "broadcast.ts, matching its sibling inventory_catalog read in the same", - "function) — properties/[id]/page.tsx (maintenance_catalog_items),", + "function) \u2014 properties/[id]/page.tsx (maintenance_catalog_items),", "inventory/page.tsx (inventory_catalog), settings/integrations/page.tsx and", "setup/pms/page.tsx (integration_providers). -read-without-error dropped", "222 -> 221 as a side effect: fetchAllRows throws on a page error instead of", @@ -54,7 +54,7 @@ "", "BURNED DOWN 2026-08-07 (second pass, same day): -read-without-error-fan-in", "14 -> 0 (all 14 Promise.all fan-ins now destructure and handle every", - "element's error, via throwIfAnyQueryFailed — isRealQueryError-filtered", + "element's error, via throwIfAnyQueryFailed \u2014 isRealQueryError-filtered", "where a .single() read's PGRST116 was already tolerated gracefully).", "-read-without-error 221 -> 171 and -discarded-result 119 -> 87 (82 more", "sites across 64 files, fanned out across four parallel batches covering", @@ -64,11 +64,11 @@ "error.tsx, a Server Action's try/catch converts the throw to its own", "{ error } return, a route handler throws bare or matches its own", "try/catch, an Inngest step.run throws to retry just that step). Added", - "lib/supabase/unwrap.ts's isRealQueryError() export in the same pass — the", + "lib/supabase/unwrap.ts's isRealQueryError() export in the same pass \u2014 the", "PGRST116-vs-real-error filter needed by nearly every Inngest .single()", "site was previously hand-rolled per call site (25 inline occurrences", "already in the tree); this is that pattern's first shared helper.", - "-read-without-error-fan-in reached 0 but was NOT promoted in this pass —", + "-read-without-error-fan-in reached 0 but was NOT promoted in this pass \u2014", "that needs its own fire-check/revert proof per .semgrep/README.md's", "promotion protocol, same as -global-table above, done separately.", "", @@ -80,7 +80,7 @@ "", "BURNED DOWN 2026-08-07 (fourth pass, same day): CI's --baseline-commit", "semgrep job failed on 9 findings the fan-in fix's read-without-error", - "rewrite incidentally surfaced as NEW relative to the PR base — the", + "rewrite incidentally surfaced as NEW relative to the PR base \u2014 the", "underlying unbounded selects (-in-list, -org-scoped, -single-parent tiers)", "already existed pre-rewrite and were already counted in these baselines,", "but touching those exact lines to add error handling made semgrep's diff", @@ -91,41 +91,41 @@ "is a correctness bug, not a display nicety) and on auto-assign-turnover's", "property-turnovers read (same unbounded-over-lifetime shape its sibling", "turnover_assignments read next to it already documented and paginated).", - "-in-list 36 -> 34, -org-scoped 101 -> 98, -single-parent 34 -> 30 — 9 net", + "-in-list 36 -> 34, -org-scoped 101 -> 98, -single-parent 34 -> 30 \u2014 9 net", "across the two files with a double fix (seed-from-amenities.ts,", "auto-assign-turnover.ts) plus the 7 single-finding files. Verified", "against the exact CI invocation: `semgrep --config .semgrep/ratchet.yml", "--baseline-commit --json` returns zero results.", "", "BURNED DOWN 2026-08-08: -read-without-error 94 -> 0 and -discarded-result", - "65 -> 0 — both fully cleared, platform-wide. 159 sites across 43 files,", + "65 -> 0 \u2014 both fully cleared, platform-wide. 159 sites across 43 files,", "fanned out across four parallel batches (~40 sites each) covering", "non-overlapping file sets, same toolkit as the prior 2026-08-07 pass", "(unwrap/unwrapList/tryUnwrap/tryUnwrapList/throwIfAnyQueryFailed/", "reportQueryError/isRealQueryError from lib/supabase/unwrap.ts). Fixed 4", "sonarjs warnings the added error-handling branches pushed over threshold", - "(cognitive-complexity/nested-control-flow) by extracting named helpers —", + "(cognitive-complexity/nested-control-flow) by extracting named helpers \u2014", "app/(dashboard)/settings/team/actions.ts's findAlreadyMemberError,", "app/api/support/chat/route.ts's resolveConversationId,", "lib/checklists/apply-master-template.ts's getOrCreateTemplateId/", "insertComposedSections, lib/inngest/functions/ownerrez/", - "ownerrez-reviews-sync.ts's notifyRevokedThrottled — net -5 vs the prior", + "ownerrez-reviews-sync.ts's notifyRevokedThrottled \u2014 net -5 vs the prior", "ceiling since the last extraction also cleared a second pre-existing", "warning for free; package.json's max-warnings lowered 191 -> 190 to lock", "it in. Both rules are now eligible for promotion to chokepoints.yml (same", - "as -global-table and potentially -read-without-error-fan-in before it) —", + "as -global-table and potentially -read-without-error-fan-in before it) \u2014", "not done in this pass; needs its own fire-check/revert proof per", ".semgrep/README.md's promotion protocol.", "", "BURNED DOWN 2026-08-08 (second pass, same day): CI's --baseline-commit", "semgrep job failed a second time, same root cause as the 2026-08-07 CI", - "fix — the read-without-error/discarded-result rewrite touched lines that", + "fix \u2014 the read-without-error/discarded-result rewrite touched lines that", "already had pre-existing unbounded selects (-in-list/-org-scoped/", "-single-parent tiers), so semgrep's diff view treated 26 of them as new", "relative to the PR base even though they were already counted here.", "Bounded all 26 for real across 22 files with an explicit .limit()", "(mostly bounded by a caller-supplied id list, a fixed per-parent set, or a", - "documented per-org page-render cap of 500) — properties/vendors/sponsors", + "documented per-org page-render cap of 500) \u2014 properties/vendors/sponsors", "list reads across page.tsx Server Components, PMS-sync property lookups", "(ownerrez/hostaway/hospitable), checklist-section and push-subscription", "reads. -in-list 23 -> 13, -org-scoped 85 -> 76, -single-parent 25 -> 18.", @@ -135,13 +135,12 @@ "unbounded-select.test.ts's separate shrink-only baseline", "(checklist-broadcast.ts, hospitable-reviews-backfill.ts,", "hostaway/initial-sync.ts, ownerrez-reviews-sync.ts) as a side effect of", - "the same .limit() additions." + "the same .limit() additions.", + "", + "PROMOTED 2026-08-11: the three Supabase error-handling rules (-discarded-result, -read-without-error, -read-without-error-fan-in) all sat at 0 and moved to chokepoints.yml, where they gate at --error across the whole tree rather than only on findings new vs. the PR base. Their keys are deleted here in the same change, per the promotion rule in .semgrep/README.md. No site was fixed in this change -- the burn-downs that got them to 0 are the 2026-08-07 and 2026-08-08 entries above; this is only the gate upgrade those burn-downs earned and never collected. Fire-checked before promoting, same protocol as -cross-tenant and -global-table: a scratch fixture with one deliberate violation per rule plus a correct control for each produced exactly 3 findings (the violations) and 0 on the controls, then was reverted. What remains in this file is the unbounded-select ladder alone: -in-list 11, -org-scoped 65, -single-parent 16." ], - "measured_at": "2026-08-10", + "measured_at": "2026-08-11", "counts": { - "fieldstay-supabase-discarded-result": 0, - "fieldstay-supabase-read-without-error": 0, - "fieldstay-supabase-read-without-error-fan-in": 0, "fieldstay-supabase-unbounded-select-in-list": 11, "fieldstay-supabase-unbounded-select-org-scoped": 65, "fieldstay-supabase-unbounded-select-single-parent": 16 diff --git a/.semgrep/chokepoints.yml b/.semgrep/chokepoints.yml index ead5bc33..23aa401d 100644 --- a/.semgrep/chokepoints.yml +++ b/.semgrep/chokepoints.yml @@ -332,3 +332,101 @@ rules: - metavariable-regex: metavariable: $T regex: ^["'](?:asset_type_standards|integration_providers|inventory_catalog|maintenance_catalog_items|oauth_states|pending_integration_links|pending_oauth_authorizations|platform_admins|platform_inventory_template_items|platform_inventory_templates|platform_seed_room_template_items|platform_seed_room_templates|platform_staff|processed_webhooks|profiles|promo_hospitable_launch_counter|stripe_processed_events|support_kb_chunks)["']$ + + # ══ PROMOTED FROM ratchet.yml 2026-08-11 ═════════════════════════════════ + # The three rules below are the Supabase error-handling family. They were + # ratchet rules with 159 + 14 live sites as recently as 2026-08-08; the + # burn-downs recorded in baseline-counts.json cleared them to 0 across the + # whole tree, so they now gate at --error everywhere instead of only on + # findings new vs. the PR base. + # + # Why these are chokepoints with NO paths.exclude: unlike the service-role + # key or Telnyx, no file "owns" the capability of discarding a PostgREST + # error — there is no legitimate owner to name. Every exemption is expressed + # purely as the handling constructs in the patterns themselves (binding the + # result, destructuring `error`, going through lib/supabase/unwrap.ts). + # That is the paths.exclude-vs-pattern-not-inside distinction in the README: + # this is handling, not ownership. + # + # Before promoting, each was confirmed to still FIRE — a rule at zero because + # it is broken is indistinguishable from one at zero because the tree is + # clean. A scratch fixture under lib/ carried one deliberate violation per + # rule plus a correct control for each; semgrep reported exactly the three + # violations and none of the three controls, then the fixture was reverted. + # Same protocol as the -cross-tenant and -global-table promotions. + # + # Their baseline-counts.json keys were deleted in the same change, per the + # promotion rule in .semgrep/README.md. + # ── A discarded PostgREST result discards the ERROR ────────────────────── + # `await supabase.from(x).update(y)` resolves with { error } instead of + # throwing, so a surrounding try/catch cannot see the failure and neither + # can Sentry. Highest-precision rule in this file. + - id: fieldstay-supabase-discarded-result + languages: [typescript] + severity: ERROR + message: >- + This write's result is discarded. PostgREST RESOLVES with { error } — it + does not throw — so a surrounding try/catch cannot catch it and nothing + reaches Sentry. Destructure { error } and branch, or use the + lib/supabase/unwrap.ts helpers. + patterns: + - pattern: await <... $S.from($T).$OP(...) ...>; + - metavariable-regex: + metavariable: $OP + regex: ^(insert|update|upsert|delete|select|rpc)$ + - pattern-not-inside: $X = await ... + - pattern-not-inside: return await ... + - pattern-not-inside: const $X = await ... + - pattern-not-inside: let $X = await ... + - pattern-not-inside: var $X = await ... + - pattern-not-inside: $F(await ...) + - pattern-not-inside: await Promise.$M(...) + + # ── A read destructured without its error ──────────────────────────────── + # `const { data } = await …` collapses "the query errored" and "zero rows" + # into the same null, so an RLS/GRANT regression renders a friendly empty + # state with nothing logged. This is the ~481-site class that + # unit/guardrails/supabase-error-handling.test.ts baselines. + - id: fieldstay-supabase-read-without-error + languages: [typescript] + severity: ERROR + message: >- + `data` destructured without `error`. A failed query and an empty table + are now indistinguishable. Use unwrap/unwrapList/tryUnwrap from + lib/supabase/unwrap.ts, or destructure { data, error } and branch. + patterns: + - pattern-either: + - pattern: "const { data } = await $Q" + - pattern: "const { data: $D } = await $Q" + - pattern: "const { data, count: $C } = await $Q" + - pattern: "const { data: $D, count: $C } = await $Q" + # Semgrep's JS object patterns match PARTIALLY — `const { data } = …` + # also matches `const { data, error } = …`. Without these two negations + # the rule reports every read in the repo, half of them already correct. + - pattern-not: "const {..., error, ...} = await $Q" + - pattern-not: "const {..., error: $E, ...} = await $Q" + - metavariable-pattern: + metavariable: $Q + pattern: <... $S.from($T) ...> + + # ── The same class, in a Promise.all fan-in ────────────────────────────── + # A separate rule because the metavariable-pattern that constrains the + # single-read form to a `.from()` chain cannot bind through an array + # destructure. Reported once per STATEMENT, not per element, and suppressed + # entirely when any element in the statement binds `error` — both are + # undercounts, never overcounts. unit/guardrails/supabase-error-handling. + # test.ts counts these PER ELEMENT, which is one concrete reason that test + # is not redundant with this ruleset. + - id: fieldstay-supabase-read-without-error-fan-in + languages: [typescript] + severity: ERROR + message: >- + `data` destructured without `error` in a Promise.all fan-in. Every query + in this batch that errors is indistinguishable from one that returned no + rows. Use unwrap/unwrapList from lib/supabase/unwrap.ts. + patterns: + - pattern-either: + - pattern: "const [..., { data: $D }, ...] = await Promise.all(...)" + - pattern: "const [..., { data }, ...] = await Promise.all(...)" + - pattern-not: "const [..., {..., error, ...}, ...] = await Promise.all(...)" + - pattern-not: "const [..., {..., error: $E, ...}, ...] = await Promise.all(...)" diff --git a/.semgrep/ratchet.yml b/.semgrep/ratchet.yml index e650fa8b..ae2c9b39 100644 --- a/.semgrep/ratchet.yml +++ b/.semgrep/ratchet.yml @@ -362,83 +362,18 @@ rules: - pattern-not-inside: $X.maybeSingle(...) - pattern-not: "$S.from($T).select($SEL, {..., head: true, ...})" - # ── A discarded PostgREST result discards the ERROR ────────────────────── - # `await supabase.from(x).update(y)` resolves with { error } instead of - # throwing, so a surrounding try/catch cannot see the failure and neither - # can Sentry. Highest-precision rule in this file. - - id: fieldstay-supabase-discarded-result - languages: [typescript] - severity: WARNING - message: >- - This write's result is discarded. PostgREST RESOLVES with { error } — it - does not throw — so a surrounding try/catch cannot catch it and nothing - reaches Sentry. Destructure { error } and branch, or use the - lib/supabase/unwrap.ts helpers. - patterns: - - pattern: await <... $S.from($T).$OP(...) ...>; - - metavariable-regex: - metavariable: $OP - regex: ^(insert|update|upsert|delete|select|rpc)$ - - pattern-not-inside: $X = await ... - - pattern-not-inside: return await ... - - pattern-not-inside: const $X = await ... - - pattern-not-inside: let $X = await ... - - pattern-not-inside: var $X = await ... - - pattern-not-inside: $F(await ...) - - pattern-not-inside: await Promise.$M(...) - - # ── A read destructured without its error ──────────────────────────────── - # `const { data } = await …` collapses "the query errored" and "zero rows" - # into the same null, so an RLS/GRANT regression renders a friendly empty - # state with nothing logged. This is the ~481-site class that - # unit/guardrails/supabase-error-handling.test.ts baselines. - - id: fieldstay-supabase-read-without-error - languages: [typescript] - severity: WARNING - message: >- - `data` destructured without `error`. A failed query and an empty table - are now indistinguishable. Use unwrap/unwrapList/tryUnwrap from - lib/supabase/unwrap.ts, or destructure { data, error } and branch. - patterns: - - pattern-either: - - pattern: "const { data } = await $Q" - - pattern: "const { data: $D } = await $Q" - - pattern: "const { data, count: $C } = await $Q" - - pattern: "const { data: $D, count: $C } = await $Q" - # Semgrep's JS object patterns match PARTIALLY — `const { data } = …` - # also matches `const { data, error } = …`. Without these two negations - # the rule reports every read in the repo, half of them already correct. - - pattern-not: "const {..., error, ...} = await $Q" - - pattern-not: "const {..., error: $E, ...} = await $Q" - - metavariable-pattern: - metavariable: $Q - pattern: <... $S.from($T) ...> - - # ── The same class, in a Promise.all fan-in ────────────────────────────── - # A separate rule because the metavariable-pattern that constrains the - # single-read form to a `.from()` chain cannot bind through an array - # destructure. Reported once per STATEMENT, not per element, and suppressed - # entirely when any element in the statement binds `error` — both are - # undercounts, never overcounts. unit/guardrails/supabase-error-handling. - # test.ts counts these PER ELEMENT, which is one concrete reason that test - # is not redundant with this ruleset. - - id: fieldstay-supabase-read-without-error-fan-in - languages: [typescript] - severity: WARNING - message: >- - `data` destructured without `error` in a Promise.all fan-in. Every query - in this batch that errors is indistinguishable from one that returned no - rows. Use unwrap/unwrapList from lib/supabase/unwrap.ts. - patterns: - - pattern-either: - - pattern: "const [..., { data: $D }, ...] = await Promise.all(...)" - - pattern: "const [..., { data }, ...] = await Promise.all(...)" - - pattern-not: "const [..., {..., error, ...}, ...] = await Promise.all(...)" - - pattern-not: "const [..., {..., error: $E, ...}, ...] = await Promise.all(...)" - - # NOTE — two rules that used to live here, fieldstay-untimed-external-fetch - # and fieldstay-role-filtered-membership-read, reached 0 findings and were + # NOTE — five rules that used to live here reached 0 findings and were # PROMOTED to chokepoints.yml, where they gate at --error across the whole - # tree. See the entries there. Their baseline-counts.json keys are removed - # with them: a ratchet entry for a rule that ratchet.yml no longer declares - # is a number nobody can lower, and check-semgrep-ratchet.mjs fails on it. + # tree instead of only on findings new vs. the PR base: + # fieldstay-untimed-external-fetch (2026-08-01) + # fieldstay-role-filtered-membership-read (2026-08-01) + # fieldstay-supabase-discarded-result (2026-08-11) + # fieldstay-supabase-read-without-error (2026-08-11) + # fieldstay-supabase-read-without-error-fan-in (2026-08-11) + # (The unbounded-select ladder's -table-scan, -cross-tenant and -global-table + # tiers were promoted the same way; they are described in the ladder section + # of .semgrep/README.md rather than listed here.) + # + # See the entries there. Their baseline-counts.json keys are removed with + # them: a ratchet entry for a rule that ratchet.yml no longer declares is a + # number nobody can lower, and check-semgrep-ratchet.mjs fails on it. diff --git a/CLAUDE.md b/CLAUDE.md index ebf8373d..836de29e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -785,6 +785,7 @@ async function geocodeZip(zip: string): Promise<{ lat: number; lng: number } | n | Hardcoded colors in components, incl. Tailwind color utilities (`text-red-500`, `hover:text-red-600`) | CSS variables (`var(--text-primary)` etc.) — use the arbitrary-value bracket syntax (`hover:text-[var(--accent-red)]`) if it needs to stay in `className` | | Hand-rolling a new tab bar | `components/ui/Tabs.tsx` | | Renaming an internal status/lookup key (e.g. `healthDot()`'s `'critical'`/`'offline'` return values) during a copy change | Only rename the display-string helper (`healthLabel()`) and hardcoded JSX text — internal keys are branched on elsewhere and renaming them silently breaks color/variant mapping | +| Giving a new table a `PRIMARY KEY (a_id, b_id)` where both are FKs to different tables | Make the PK single-column (drop the derivable FK — a child of a property-scoped parent already knows its property). PostgREST reads that shape as a many-to-many JUNCTION and starts offering a second embed path between the two parents, so EVERY pre-existing `.select('*, parent(...)')` between them breaks with HTTP 300 / `PGRST201` — queries that never mention your new table. This shipped on 2026-08-10 and broke four call sites (inventory page, `inventory/actions.ts`, `lib/notifications.ts`, `lib/support/account-tools.ts`); only one had a test. A `UNIQUE` on the same pair is fine — the detection keys on the PRIMARY KEY. Enforced by `scripts/check-db-invariants.mjs` check 10 | | Creating a table without RLS | Always `ENABLE ROW LEVEL SECURITY` + policies | | Multiple Inngest steps creating same record | Check `source_reference_id` first | | `any` type | Explicit interface or generic | @@ -1399,13 +1400,18 @@ following them stops being a memory test. Five layers, checked in CI via with no `AbortSignal`, `void` on a lazy PostgREST builder (the request is never sent), `getPublicUrl()` on the three private buckets, and the `memberships`/`work_order_notes`/`assigned_crew_id` names that do not - exist. The last two were PROMOTED from `ratchet.yml` on 2026-08-01 once - their counts hit 0 — that promotion is the ratchet's purpose, and it - requires deleting the rule's `baseline-counts.json` key in the same - change. + exist, and — PROMOTED 2026-08-11 — the whole Supabase error-handling + family: a discarded write result, `data` destructured without `error`, + and the same in a `Promise.all` fan-in. Promotion is the ratchet's + purpose, and it requires deleting the rule's `baseline-counts.json` key + in the same change plus a fire-check (violation + a correct CONTROL, + confirm the rule catches the first and not the second, revert) — a rule + at 0 because it is BROKEN looks identical to one at 0 because the tree + is clean. - `.semgrep/ratchet.yml` — a defect class with many legitimate owners and - hundreds of live sites (discarded write results, `data` destructured - without `error`, and the unbounded-`.select()` ladder below). Gated on + many live sites. As of 2026-08-11 it holds the unbounded-`.select()` + ladder below and nothing else (92 findings); everything that ever + reached 0 has been promoted out. Gated on `--baseline-commit` (only findings NEW vs. the PR base fail) plus `.semgrep/baseline-counts.json`, a committed per-rule count that `scripts/check-semgrep-ratchet.mjs` allows to move only DOWN. Lock in a diff --git a/FUTURE_REMEDIATION.md b/FUTURE_REMEDIATION.md index f198b9a3..c67f49d4 100644 --- a/FUTURE_REMEDIATION.md +++ b/FUTURE_REMEDIATION.md @@ -1490,3 +1490,54 @@ hygiene: `PLANS.hosts` has `STRIPE_PRICE_HOSTS_MONTHLY` / `_ANNUAL`, and missing id surfaces as a failed checkout at the moment a trial user picks the plan. The Hosts tier is now advertised on `/`, `/hosts` and `/strops`'s JSON-LD. An armed drift check is what would say so before a customer does. + +--- + +## 32. Dynamic PAR: replace the flat buffer, and size par to a restock cycle + +Deliberate follow-up to the dynamic PAR engine, agreed with the product owner +2026-08-10 while porting it. The engine as shipped is correct and useful; +these are upgrades to its *historical* branch only. `static` and +`smart_formula` are unaffected, so nothing here blocks the feature. + +**1. The flat buffer ignores variability.** `historicalPar()` applies a fixed +`HISTORICAL_BUFFER = 0.20` regardless of how erratic the item's consumption +is, so a steadily-consumed item and a wildly-varying one get the same 20%. +Standard safety-stock sizing scales the buffer with the standard deviation of +demand, not the mean: + +```text +par = mean_demand + Z x sigma (Z = 1.65 for ~95% service, 2.33 for ~99%) +``` + +`inventory_consumption_stats` currently stores only `avg_rate_per_guest_night` +and `sample_count`, so there is no sigma to use. Welford's online algorithm +gets it from a running `M2` column without retaining samples — about ten lines +and one migration adding two columns. No dependency: this is a formula, not a +library. + +**2. A plain mean never forgets.** A count from six months ago weighs the same +as last week's, so seasonality and a changed guest mix both wash out. An +exponentially weighted moving average (`new = alpha*obs + (1-alpha)*old`, +alpha ~ 0.3) tracks them and removes any reason for `sample_count` to grow +without bound. + +**3. Par should cover a restock CYCLE, not one stay.** This is the conceptual +one, and the product owner's call on the numbers: + +- coverage target: **3–4 stays**, not the single average stay + `historicalPar()` assumes today +- restock lead time: **2.5 days**, chosen to err cautious + +The classic form is `par = rate x lead_time + Z x sigma x sqrt(lead_time)`. +FieldStay already knows lead time is real — it has purchase orders and Kroger +cart automation — so a par that means "enough until I can restock" is +expressible, whereas "enough for one stay" is what the formula currently +computes. + +**Sequencing.** Do this AFTER the PAR port lands, and consider doing it with +or after the deferred Pass 4 (per-reservation guest counts): Pass 4 changes +the guest-night denominator from the `max_guests x avg_stay_length` proxy to +booking actuals, and changing the denominator changes the distribution whose +variance item 1 would be measuring. Landing them in the wrong order means +computing sigma over a proxy and then invalidating it. diff --git a/docs/PAR_ENGINE_PORT_STATE.md b/docs/PAR_ENGINE_PORT_STATE.md new file mode 100644 index 00000000..dd02be26 --- /dev/null +++ b/docs/PAR_ENGINE_PORT_STATE.md @@ -0,0 +1,347 @@ +# Dynamic PAR Engine — port state & open design + +Handoff written 2026-08-10 at the end of a long session. Everything below is +verified against the live code and databases unless explicitly marked as a +decision or an open question. + +**Read this first, then `FUTURE_REMEDIATION.md` items 31 and 32.** + +--- + +## 1. What this is, and why it matters + +A self-adjusting inventory par system. In the product owner's words: + +> I want to be able to go into the templates in admin, put in a FieldStay +> standard inventory template with pars already set based on average usage, +> and the system set up so that when the template is used — or when **any** +> inventory template is created and used, not just that one — the par system +> self-adjusts based on previous inventory counts, max guests, and rooms. + +That "not just that one" is the load-bearing requirement, and the +implementation genuinely satisfies it. Par config propagates: + +``` +inventory_catalog (platform/admin master) + └─> org_inventory_catalog (seeded per org, lib/inventory/seed-org-catalog.ts) + └─> inventory_template_items (ANY org template — inherits unless overridden) + └─> inventory_items (property level — inherits; recompute fires on apply) +``` + +So an admin setting a catalog item to `smart` makes every downstream template +and property item self-adjust, including templates a PM builds themselves. + +--- + +## 2. Where the code is RIGHT NOW + +Branch `claude/hostile-code-audit-rbz3f8`, pushed. Two relevant commits: + +| Commit | Contents | +|---|---| +| `722c17ba` | `lib/inventory/par-engine.ts` + `unit/inventory/par-engine.test.ts` (14 tests, green) | +| `d584a2a1` | The schema migration, applied to BOTH databases + `FUTURE_REMEDIATION` 32 | + +### Applied to the live databases + +`supabase/migrations/20260810214329_dynamic_par_engine_schema.sql` is applied +to **production** (`vpmznjktllhmmbfnxuvk`) and **E2E** +(`syhthijeqlnltufdawyb`). + +Post-apply verification against production: `par_mode` present on 5 tables, +`inventory_consumption_stats` exists with RLS enabled, 1 policy, 3 indexes, +and **0 of 147 catalog rows are non-static** — i.e. no behaviour changed. + +> **That verification was not sufficient, and the follow-up migration +> `20260811020000_fix_par_stats_junction_ambiguity.sql` is why.** It checked +> that the new objects existed; it did not check what they did to objects that +> already existed. `inventory_consumption_stats` was created with +> `PRIMARY KEY (property_id, inventory_item_id)` — two single-column FKs to two +> different tables, which is exactly PostgREST's signature for a many-to-many +> JUNCTION table. PostgREST began offering a second path between +> `inventory_items` and `properties`, so every pre-existing embed between them +> started returning HTTP 300 / `PGRST201`. Four live call sites: the inventory +> page, `inventory/actions.ts`, `lib/notifications.ts` and +> `lib/support/account-tools.ts`. **One** E2E test caught it; the other three +> were broken in production with every other gate green. +> +> **The stats table's grain changed as a result — pass 2 must assume the new +> shape.** `property_id` is GONE and the PK is `inventory_item_id` alone. +> Nothing was lost: `inventory_items` is already property-level, so the column +> was derivable from its sibling and could drift out of agreement with it. A +> single-column PK also cannot be re-read as a junction, so this can't regress +> on this table. `org_id` stays — equally derivable, but load-bearing for RLS. +> +> A new invariant in `scripts/check-db-invariants.mjs` (check 10, backed by +> `public.accidental_junction_tables()`) now fails CI on any table with this +> shape, with an empty allowlist. The rule this encodes generalises past PAR: +> **adding a table can break queries that never mention it.** + +> **The ledger/file parity trap — read before applying the next migration.** +> There is no Supabase CLI and no `SUPABASE_ACCESS_TOKEN` in the agent +> environment, so migrations must go through the MCP `apply_migration`. **MCP +> assigns its own version and a different one per call.** The same migration +> landed as `20260810214329` on E2E and `20260810214410` on prod, while the +> local file said `20260810120000` — three versions for one migration, which +> is the exact drift that put production at 36/35 divergences in audit H10. +> +> It was reconciled by hand: prod's ledger row was UPDATEd to E2E's version +> and the local file renamed to match. **Do the same for the next two:** apply +> to E2E, read the recorded version, apply to prod, normalise prod's row to +> E2E's version, rename the local file. Then `pnpm run check:migration-ledger:prod`. + +### Parked, NOT applied, NOT committed + +These two are in the session scratchpad only (they will be lost — regenerate +them from the source branch if needed): + +- `20260810130000_dynamic_par_engine_pipeline.sql` +- `20260810140000_dynamic_par_engine_platform_template_rpc.sql` + +Source branch for everything: **`origin/claude/migration-reconciliation-5is9pq`** +(misleadingly named — it was reused after PR #539; it actually contains the +5 PAR commits). Original spec files: `CLAUDE_PAR_1/2/3.md`. + +> The RPC migration's original version `20260730140000` **collides** with +> main's already-applied `20260730140000_atomic_subscription_plan_update.sql`. +> Left as-is, `db push` would silently skip the PAR RPC and the feature would +> ship with its RPC missing. It has been renumbered to `20260810140000`; +> keep it renumbered. + +--- + +## 3. Four defects in the source branch — fix during the port + +The feature's design is sound. Its data-layer plumbing predates the +2026-08-07/08 hardening passes and will fail current CI gates. + +1. **The recompute writes nothing, silently.** `inventory-par-recompute.ts` + does `.upsert(changedRows, { onConflict: 'id' })` where `changedRows` is + `{id, par_level, par_resolved_at}`. `inventory_items` has three NOT NULL + columns with no default — `name`, `org_id`, `property_id` — and Postgres + validates NOT NULL when it forms the tuple, *before* it detects the + conflict. Every recompute raises `23502`. The result is discarded, so it + fails silently while returning `items_changed: changedRows.length`. + **The engine would never update a single par level.** Fix: carry the + required columns, or use a bulk `UPDATE ... FROM (VALUES ...)` RPC. + +2. **Three unbounded `.select()`s in a new `lib/inngest/` file.** + `unbounded-select.test.ts` grandfathers a BASELINE; new files must comply. + `fetch-scope` is a real defect — with `property_id` omitted it is an + org-wide read that truncates at 1000 properties. + +3. **Four discarded read/write results.** `discarded-result` and + `read-without-error` were both at **0** in `.semgrep/baseline-counts.json`, + so these pushed both off zero, which the ratchet forbade outright. + **This got stricter on 2026-08-11:** both rules — plus + `read-without-error-fan-in` — were PROMOTED out of `ratchet.yml` into + `.semgrep/chokepoints.yml`, where they gate at `--error` across the whole + tree instead of only on findings new vs. the PR base. There is no longer a + baseline number to go up; any one of these four sites now fails CI + outright, and there is no `nosemgrep` or `paths.exclude` escape (see the + "Never silence a ratchet" rule in `.semgrep/README.md`). Fix all four with + the `lib/supabase/unwrap.ts` helpers as part of pass 2 — not afterwards. + +4. **`PropertyRow` lies about nullability.** It types `bedrooms`/`max_guests` + as non-null `number` behind an `as PropertyRow[]` cast; both are nullable + in the DB. The resolver guards at runtime, so it is a wrong type rather + than a live bug. + +### One spec task is dead + +PAR_2 Task 6B wires consumption capture into `approveInventoryCount` on the +inventory-count **draft** path. That whole family was dropped by +`20260804125424_drop_inventory_count_drafts.sql` and `approveInventoryCount` +no longer exists. **Drop task 6B.** Task 6A's path +(`handleInventoryCountSubmitted`) is alive and becomes the only consumption +source. + +### An expired constraint (good news) + +PAR_1 and PAR_2 both say *"DO NOT run `db push` or `apply_migration` — +migration reconciliation is unresolved."* That is why this feature sat frozen. +Reconciliation completed 2026-08-03 at 313/313 parity with a CI gate. **The +constraint no longer applies.** + +--- + +## 4. Type-generation ordering (this blocks the port) + +`lib/supabase/server.ts` wires `createServerClient`, so `.select('*')` +infers from `types/database.generated.ts`, which is generated from the live +schema. Sequence is therefore forced: + +``` +apply migration -> regenerate types/database.generated.ts -> update types/database.ts -> code compiles +``` + +`types/database.ts` was deliberately NOT updated in `d584a2a1`: nothing +selects the new columns yet, and declaring them before the generated file +knows about them breaks `.select('*')` inference (it did — `app/(dashboard)/ +inventory/page.tsx`'s `type InventoryItemRow = InventoryItem & {…}` failed +tsc). The schema is now applied, so regeneration is unblocked. Note the +regenerated file is ~6,000 lines and will consume a lot of context. + +--- + +## 5. The engine as built + +`lib/inventory/par-engine.ts` — pure, synchronous, total (never throws). + +``` +par_mode 'static' -> stored par_level returned unchanged +par_mode 'smart' -> 1. historical, if auto_adjust AND sample_count >= 3 + 2. else smart-group formula +``` + +- `smart_formula` = `ceil(base_qty × property[multiplier] × (1 + buffer))` +- `historical` = `ceil(rate_per_guest_night × max_guests × avg_stay_length × 1.20)`, floor 2 + +| Group | Scales by | Buffer | +|---|---|---| +| `bathroom_essential` | `properties.bathrooms` | +15% | +| `bedroom_essential` | `properties.bedrooms` | +20% | +| `guest_consumable` | `properties.max_guests` | +10% | + +**Both branches compute ONE STAY.** This is the single most important fact for +the sheet work below: `base_qty` must mean *per unit, per stay*. If coverage +is baked into `base_qty` instead, then the moment an item reaches 3 samples +the historical branch takes over and computes ~3× smaller — pars would +visibly collapse on exactly the items that are working best. + +Deferred to **Pass 4** (per the spec, do not build now): per-reservation guest +counts — add `bookings.guest_count` from OwnerRez/Hospitable mappers and +switch both the consumption recorder and `historicalPar` off the +`max_guests × avg_stay_length` proxy **in the same pass**, since the proxy +must change on both sides together. + +--- + +## 6. The seed sheet + +`fieldstay_inventory_seed_addition.xlsx` / `.csv` — 133 rows. +Columns: `Name, Category, Unit, Par Level, Per Bedroom, Per Bathroom, +Description, Active, Brand-Essentials, Brand-Standard, Brand-Premium`. + +Data is clean: **zero** rows set both Per columns, every row has some par. + +### Mapping to the schema + +| Sheet | Schema | +|---|---| +| `Per Bathroom` filled (14 rows) | `par_mode='smart'`, `smart_group='bathroom_essential'`, `base_qty` = value | +| `Per Bedroom` filled (10 rows) | `par_mode='smart'`, `smart_group='bedroom_essential'`, `base_qty` = value | +| neither (109 rows) | `par_mode='static'`, `default_par_level` = `Par Level` | + +Two rows carry both a `Par Level` and a Per value (Disposable Razor, +Shampoo Bulk). Not a conflict — the schema keeps `default_par_level` as the +static fallback alongside smart config. + +### The owner's methodology — CONFIRMED, and not uniform + +This was never written on the sheet. Captured verbatim: + +- **Towels and such** — per guest per stay, plus a little extra, *because they + are laundered*. **No multi-stay coverage.** +- **Sheets and pillows type items** — per bed, plus a spare. +- **Welcome pack** — per stay, regardless of guest count. +- **K-cups** — "not actually enough quantity, but supposed to be enough for + 3 stays." +- **Toilet paper type items** — same, 3 stays. + +The numbers decode against this exactly: + +| Item | Sheet | Decodes as | +|---|---|---| +| Toilet Paper | 18 | 1/guest × 6 guests × **3 stays** ✔ | +| Bath Towels | 12 | 2/guest × 6 guests, **no coverage** (laundered) ✔ | +| Coffee Welcome Pack | 4 | 1/stay × **4 stays** ✔ | +| Pool Towels | 10 | ~1.5/guest × 6, no coverage ✔ | + +### Proposed `Per Guest` column (per guest, PER STAY — no coverage baked in) + +| Item | Per Guest | Notes | +|---|---|---| +| Bath Towels | 2 | laundered — no coverage | +| Pool Towels | 1.5 | laundered | +| Beach Towels | 1 | laundered | +| Toilet Paper | 1 | **consumable — needs 3× coverage** | +| Paper Cups | 4 | consumable | +| Coffee K-Cup | 3 | consumable | +| Drinking Glasses | 2 | reused | +| Wine Glasses | 1 | reused | +| Outdoor Drinkware | 1 | reused | + +**Excluded and why:** Coffee Welcome Pack and (probably — CONFIRM) Local Snack +Assortment are *per stay*, not per guest. All of Cleaning, Laundry, +Maintenance & Safety and the Kitchen durables are per-turnover or equipment — +they scale with the property, not headcount. + +**Pack-unit items are unresolved.** Bottled Water, Napkins, Chocolates, +Sugar/Sweetener and Paper Plates are measured in *packs*, so per-guest lands +at fractions like 0.08 and `ceil()` flattens every property onto "1 pack". +Left static. Fixing this means a unit change (bottles, not packs) — a data +decision for the owner. + +--- + +## 7. Design gaps the sheet exposed + +The engine models **one** attribute (what an item scales by). The owner's +methodology needs **three**. All three are cheap to add now while the schema +is days old; each is an enum-and-column migration later. + +1. **A per-stay dimension.** A welcome pack scales with turnover frequency, + which none of the three groups express. Proposed fourth group + `stay_essential`, `base_qty` 1, resolving to `ceil(coverage_stays × base_qty)`. + Today the only encoding is `static`, where the number *is* the coverage + count — which is what the owner did, but it means the coverage factor is + hand-typed into every such item and item 32's setting will never reach them. + +2. **Consumable vs reused — the gate on coverage.** This is the important one. + If item 32 multiplies every smart par by 3–4 stays, toilet paper goes + 7 → 23 (right) but **bath towels go 14 → 49** (absurd — they are + laundered). Proposed: a `consumable` boolean, with + `par = base × multiplier × (consumable ? coverage_stays : 1)`. + +3. **Rotation / spare for linens.** "Per bed plus a spare" is a third shape — + neither scaling nor coverage. May be expressible as `base_qty` rounding + (2 sets per bed rather than 1), or may want its own term. **Open.** + +--- + +## 8. Open questions for the owner + +1. Is **Local Snack Assortment** per-stay (like the welcome pack) or per-guest? +2. Add the fourth `stay_essential` group? (recommended — cheap now) +3. Add the `consumable` flag? (**strongly** recommended — without it, item 32 + would tell PMs to stock 49 bath towels) +4. Do the pack-unit items get a unit change so they can scale? +5. Is the seed sheet an **addition** to the existing 147 catalog items, or a + **replacement**? Name-overlap has not been checked yet. +6. **Sequencing:** land item 32's coverage multiplier *before* flipping + consumables to smart? Otherwise toilet paper visibly drops 18 → 7 in the + UI until item 32 ships. Reusables are unaffected either way. +7. Brand tiers (`Brand-Essentials/Standard/Premium`) have no schema support — + only a singular `preferred_brand` exists. All three columns are empty in + the current file. Separate pass. + +--- + +## 9. Suggested order of work + +1. Regenerate `types/database.generated.ts`, update `types/database.ts`. +2. Decide gaps 1–3 above; if adding the group/flag, do it as ONE migration now. +3. Apply + reconcile the pipeline and RPC migrations (see the parity trap in §2). +4. Port PAR_2 (recompute + consumption recorder + wiring), **fixing all four + defects in §3 and dropping task 6B**. +5. Port PAR_3 (admin catalog UI, admin templates, org master list, par-levels + browser + explainability). +6. Load the seed sheet. +7. Full gate: `tsc`, `vitest run`, `npm run lint` (ratchet 181), `check:ui-classes`, + semgrep chokepoints + ratchet, `next build`, `check:migration-ledger:prod`. + +Verification standard used throughout this session: **canary every guardrail +by breaking the thing it protects**, and verify claims against the live +database rather than the spec — every spec in this repo has had at least one +premise that expired. diff --git a/lib/inventory/par-engine.ts b/lib/inventory/par-engine.ts new file mode 100644 index 00000000..addb54df --- /dev/null +++ b/lib/inventory/par-engine.ts @@ -0,0 +1,128 @@ +/** + * Dynamic PAR resolution engine — pure, synchronous, dependency-free. + * + * The database stores CONFIG (par_mode, smart_group, base_qty, auto_adjust); + * this module holds the FORMULAS. inventory_items.par_level is a + * server-maintained cache of resolvePar()'s output for 'smart' items and a + * plain manual value for 'static' items. Recompute happens on write (Inngest + * `inventory/par-recompute-requested`, pass 2), never on read — the crew PWA, + * PO generation, and low-stock checks keep reading par_level untouched. + * + * Resolution priority for par_mode = 'smart': + * 1. Historical consumption — once >= HISTORICAL_MIN_SAMPLES count-derived + * samples exist AND the item has auto_adjust = true. + * 2. Smart-group formula — ceil(base_qty × property[multiplier] × (1 + buffer)). + * par_mode = 'static' short-circuits: the stored par_level is returned as-is. + */ + +// The two enum unions live in types/database.ts with every other Postgres +// enum, because scripts/check-type-drift.mjs parses that file to diff them +// against the live enum labels. Imported back here (type-only, so this module +// stays runtime-dependency-free) and re-exported, so existing callers that +// import them from the engine keep working. +import type { ParMode, ParSmartGroup } from '@/types/database' +export type { ParMode, ParSmartGroup } + +export interface SmartGroupSpec { + /** properties column the formula scales by */ + multiplierKey: 'bathrooms' | 'bedrooms' | 'max_guests' + /** safety buffer as a fraction, e.g. 0.15 = +15% */ + buffer: number + label: string +} + +/** Global defaults. Changing a value here requires a recompute broadcast + * (pass 2) to refresh cached par_levels — never a data migration. */ +export const PAR_SMART_GROUPS: Record = { + bathroom_essential: { multiplierKey: 'bathrooms', buffer: 0.15, label: 'Bathroom essential (scales with bathrooms)' }, + bedroom_essential: { multiplierKey: 'bedrooms', buffer: 0.20, label: 'Bedroom essential (scales with bedrooms)' }, + guest_consumable: { multiplierKey: 'max_guests', buffer: 0.10, label: 'Guest consumable (scales with max guests)' }, +} + +/** Historical engine only activates with at least this many consumption samples. */ +export const HISTORICAL_MIN_SAMPLES = 3 +/** Safety buffer applied on top of the historical expected usage. */ +export const HISTORICAL_BUFFER = 0.20 +/** Historical par never resolves below this floor. */ +export const HISTORICAL_FLOOR = 2 + +export interface ParPropertyContext { + bathrooms: number | null + bedrooms: number + max_guests: number + /** properties.avg_stay_length — nights of a typical stay */ + avg_stay_length: number | null +} + +export interface ParItemConfig { + par_mode: ParMode + smart_group: ParSmartGroup | null + base_qty: number + /** current stored par_level — returned unchanged for 'static' items */ + par_level: number + auto_adjust: boolean +} + +export interface ParConsumptionStats { + avg_rate_per_guest_night: number + sample_count: number +} + +export type ParSource = 'static' | 'historical' | 'smart_formula' + +export interface ResolvedPar { + par: number + source: ParSource +} + +function smartFormulaPar(config: ParItemConfig, property: ParPropertyContext): number { + // The CHECK constraint guarantees smart rows carry a group, but the resolver + // must still be total: a malformed row degrades to the stored value rather + // than throwing inside an Inngest step. + if (!config.smart_group) return Math.max(Math.ceil(config.par_level), 1) + const spec = PAR_SMART_GROUPS[config.smart_group] + const raw = property[spec.multiplierKey] + // bathrooms is nullable (numeric, half-baths allowed) — a property with no + // metadata yet resolves against 1 unit so a template apply never writes 0. + const multiplier = typeof raw === 'number' && raw > 0 ? raw : 1 + return Math.max(Math.ceil(config.base_qty * multiplier * (1 + spec.buffer)), 1) +} + +function historicalPar(stats: ParConsumptionStats, property: ParPropertyContext): number { + const guests = property.max_guests > 0 ? property.max_guests : 2 + const nights = property.avg_stay_length && property.avg_stay_length > 0 ? property.avg_stay_length : 3 + const expected = stats.avg_rate_per_guest_night * guests * nights + return Math.max(Math.ceil(expected * (1 + HISTORICAL_BUFFER)), HISTORICAL_FLOOR) +} + +export function resolvePar( + config: ParItemConfig, + property: ParPropertyContext, + stats: ParConsumptionStats | null +): ResolvedPar { + if (config.par_mode === 'static') { + return { par: config.par_level, source: 'static' } + } + if (config.auto_adjust && stats && stats.sample_count >= HISTORICAL_MIN_SAMPLES && stats.avg_rate_per_guest_night > 0) { + return { par: historicalPar(stats, property), source: 'historical' } + } + return { par: smartFormulaPar(config, property), source: 'smart_formula' } +} + +export interface ParConfigInput { + par_mode: ParMode + smart_group: ParSmartGroup | null + base_qty: number +} + +/** Coerces client-submitted par config into a DB-valid shape: static rows + * never carry a group; smart rows must name a valid group or degrade to + * static; base_qty is clamped positive. Server actions call this so the + * smart_group_matches_mode CHECK can never reject a write. */ +export function normalizeParConfig(input: ParConfigInput): ParConfigInput { + const base_qty = Number.isFinite(input.base_qty) && input.base_qty > 0 ? input.base_qty : 1 + if (input.par_mode === 'smart' && input.smart_group && input.smart_group in PAR_SMART_GROUPS) { + return { par_mode: 'smart', smart_group: input.smart_group, base_qty } + } + return { par_mode: 'static', smart_group: null, base_qty } +} diff --git a/scripts/check-db-invariants.mjs b/scripts/check-db-invariants.mjs index 156a0ab8..dda8bdec 100644 --- a/scripts/check-db-invariants.mjs +++ b/scripts/check-db-invariants.mjs @@ -184,6 +184,28 @@ const ORG_ID_FK_EXCEPTIONS = new Set([ 'maintenance_schedule_templates', ]) +// ── PostgREST junction-table allowlist (check 10) ────────────────────────── +// A table whose PRIMARY KEY is exactly two single-column FKs to two different +// tables is read by PostgREST as a many-to-many JUNCTION, and it then offers a +// second embedding path between those two parents. Any pre-existing +// `.select('*, parent(...)')` between them starts returning HTTP 300 / +// PGRST201 "Could not embed because more than one relationship was found". +// +// This is not theoretical: 20260810214329_dynamic_par_engine_schema.sql created +// inventory_consumption_stats with PRIMARY KEY (property_id, inventory_item_id) +// and broke four live call sites between inventory_items and properties — the +// inventory page, inventory/actions.ts, lib/notifications.ts and +// lib/support/account-tools.ts. One E2E test caught it; the other three were +// broken in production with every other check green. Fixed by +// 20260811020000_fix_par_stats_junction_ambiguity.sql. +// +// A GENUINE join table belongs here — the shape is correct for it and the +// many-to-many embed is the point. Everything else is the bug above. EMPTY +// today, and shrink-only in spirit: adding an entry means "yes, I want +// PostgREST to treat this as a join table", which is a design decision, not a +// suppression. Verify the embeds you are making ambiguous before adding one. +const JUNCTION_TABLE_ALLOWLIST = new Set([]) + const res = await fetch(new URL('/rest/v1/rpc/db_invariant_report', url), { method: 'POST', headers: { @@ -396,6 +418,60 @@ if (staleOrgFkAllowlist.length > 0) { ) } +// ── 10. Accidental PostgREST junction tables ────────────────────────────── +// Separate RPC from db_invariant_report() so this gate works against a project +// that has not yet had the report function extended; a missing function is a +// hard failure, never a silent skip. +const junctionRes = await fetch(new URL('/rest/v1/rpc/accidental_junction_tables', url), { + method: 'POST', + headers: { + apikey: key, + authorization: `Bearer ${key}`, + 'content-type': 'application/json', + }, + body: '{}', +}) + +if (!junctionRes.ok) { + console.error(`accidental_junction_tables RPC failed: HTTP ${junctionRes.status}`) + console.error( + 'Has supabase/migrations/20260811020000_fix_par_stats_junction_ambiguity.sql ' + + 'been applied to this project?' + ) + process.exit(1) +} + +const junctions = await junctionRes.json() +if (!Array.isArray(junctions)) { + console.error('accidental_junction_tables() did not return a list.') + process.exit(1) +} + +const unexpectedJunctions = junctions.filter((j) => !JUNCTION_TABLE_ALLOWLIST.has(j.junction_table)) +const staleJunctionAllowlist = [...JUNCTION_TABLE_ALLOWLIST].filter( + (t) => !junctions.some((j) => j.junction_table === t) +) +if (unexpectedJunctions.length > 0) { + const described = unexpectedJunctions + .map((j) => `${j.junction_table} (PK ${(j.pk_columns ?? []).join(' + ')} -> ${(j.parents ?? []).join(', ')})`) + .join('; ') + failures.push( + `Tables PostgREST will read as many-to-many junctions: ${described}\n` + + ' A PK of exactly two single-column FKs to two different tables makes ' + + "EVERY existing embed between those two parents ambiguous (HTTP 300 / " + + 'PGRST201), including ones written long before this table existed. If ' + + 'the second FK column is derivable from the first, drop it and make the ' + + 'PK single-column. If this really is a join table, add it to ' + + 'JUNCTION_TABLE_ALLOWLIST in scripts/check-db-invariants.mjs.' + ) +} +if (staleJunctionAllowlist.length > 0) { + failures.push( + `Stale JUNCTION_TABLE_ALLOWLIST entries (table no longer has that shape, or was dropped): ${staleJunctionAllowlist.join(', ')}\n` + + ' Remove them from scripts/check-db-invariants.mjs — the allowlist only shrinks.' + ) +} + // ── Verdict ─────────────────────────────────────────────────────────────── if (failures.length > 0) { console.error(`DB invariant check FAILED (${failures.length} finding${failures.length === 1 ? '' : 's'}):\n`) @@ -407,5 +483,6 @@ console.log( 'DB invariants OK — RLS on every table, no unexpected deny-all tables, ' + 'all FK columns indexed, zero anon grants, all dedup-key columns indexed ' + 'unique, every member-facing policy backed by its GRANT, no memberless ' + - 'orgs, every storage policy org-scoped, every org_id column FK-backed.' + 'orgs, every storage policy org-scoped, every org_id column FK-backed, ' + + 'no accidental PostgREST junction tables.' ) diff --git a/scripts/check-type-drift.mjs b/scripts/check-type-drift.mjs index 47033a9b..0f97081a 100644 --- a/scripts/check-type-drift.mjs +++ b/scripts/check-type-drift.mjs @@ -119,6 +119,8 @@ const ENUM_MAP = { member_role: 'MemberRole', org_plan: 'OrgPlan', org_plan_status: 'OrgPlanStatus', + par_mode: 'ParMode', + par_smart_group: 'ParSmartGroup', po_status: 'PoStatus', priority_level: 'PriorityLevel', property_type: 'PropertyType', diff --git a/supabase/migrations/20260810214329_dynamic_par_engine_schema.sql b/supabase/migrations/20260810214329_dynamic_par_engine_schema.sql new file mode 100644 index 00000000..8a3adebc --- /dev/null +++ b/supabase/migrations/20260810214329_dynamic_par_engine_schema.sql @@ -0,0 +1,154 @@ +-- Dynamic PAR engine, pass 1 — configuration columns across the catalog → +-- template → property-item chain, plus the consumption-stats table the +-- historical engine (pass 2) will populate. +-- +-- Design (see lib/inventory/par-engine.ts): +-- par_mode 'static' — par_level is a manually-set integer; the engine never +-- touches it. This is the default, so every existing row behaves exactly +-- as it does today. +-- par_mode 'smart' — par_level becomes a server-maintained CACHE. The +-- resolver computes it from the row's smart_group + base_qty against the +-- property's bedrooms/bathrooms/max_guests, or from historical +-- consumption once enough samples exist. Multipliers and buffers live in +-- code (PAR_SMART_GROUPS in lib/inventory/par-engine.ts), NOT in the +-- database, so tuning a global default never needs a data migration — +-- just a recompute broadcast. + +-- ── Enums ─────────────────────────────────────────────────────────────────── + +DO $$ BEGIN + CREATE TYPE par_mode AS ENUM ('static', 'smart'); +EXCEPTION WHEN duplicate_object THEN NULL; END $$; + +DO $$ BEGIN + CREATE TYPE par_smart_group AS ENUM ( + 'bathroom_essential', -- scales with properties.bathrooms + 'bedroom_essential', -- scales with properties.bedrooms + 'guest_consumable' -- scales with properties.max_guests + ); +EXCEPTION WHEN duplicate_object THEN NULL; END $$; + +-- ── Platform master catalog (admin panel: /admin/inventory-catalog) ───────── +-- base_qty is the per-unit-of-multiplier baseline (e.g. 2 rolls PER bathroom). +-- default_par_level (existing) remains the static fallback / pre-fill. + +ALTER TABLE public.inventory_catalog + ADD COLUMN IF NOT EXISTS par_mode par_mode NOT NULL DEFAULT 'static', + ADD COLUMN IF NOT EXISTS smart_group par_smart_group, + ADD COLUMN IF NOT EXISTS base_qty numeric NOT NULL DEFAULT 1 CHECK (base_qty > 0); + +-- A smart row must say which group it scales by; a static row must not carry +-- a stale group. Enforced at the catalog roots so bad config can't propagate. +DO $$ BEGIN + ALTER TABLE public.inventory_catalog + ADD CONSTRAINT inventory_catalog_smart_group_matches_mode + CHECK ((par_mode = 'smart') = (smart_group IS NOT NULL)); +EXCEPTION WHEN duplicate_object THEN NULL; END $$; + +-- ── Org's editable catalog copy (Templates Hub master list) ───────────────── + +ALTER TABLE public.org_inventory_catalog + ADD COLUMN IF NOT EXISTS par_mode par_mode NOT NULL DEFAULT 'static', + ADD COLUMN IF NOT EXISTS smart_group par_smart_group, + ADD COLUMN IF NOT EXISTS base_qty numeric NOT NULL DEFAULT 1 CHECK (base_qty > 0); + +DO $$ BEGIN + ALTER TABLE public.org_inventory_catalog + ADD CONSTRAINT org_inventory_catalog_smart_group_matches_mode + CHECK ((par_mode = 'smart') = (smart_group IS NOT NULL)); +EXCEPTION WHEN duplicate_object THEN NULL; END $$; + +-- ── Platform inventory templates (admin panel: /admin/inventory-templates) ── +-- Carried per template item so the admin can override the catalog default per +-- template (e.g. the same "Bath Towels" item static in a budget template but +-- smart/guest_consumable in the standard template). + +ALTER TABLE public.platform_inventory_template_items + ADD COLUMN IF NOT EXISTS par_mode par_mode NOT NULL DEFAULT 'static', + ADD COLUMN IF NOT EXISTS smart_group par_smart_group, + ADD COLUMN IF NOT EXISTS base_qty numeric NOT NULL DEFAULT 1 CHECK (base_qty > 0); + +DO $$ BEGIN + ALTER TABLE public.platform_inventory_template_items + ADD CONSTRAINT platform_inv_tpl_items_smart_group_matches_mode + CHECK ((par_mode = 'smart') = (smart_group IS NOT NULL)); +EXCEPTION WHEN duplicate_object THEN NULL; END $$; + +-- ── Org inventory templates ───────────────────────────────────────────────── + +ALTER TABLE public.inventory_template_items + ADD COLUMN IF NOT EXISTS par_mode par_mode NOT NULL DEFAULT 'static', + ADD COLUMN IF NOT EXISTS smart_group par_smart_group, + ADD COLUMN IF NOT EXISTS base_qty numeric NOT NULL DEFAULT 1 CHECK (base_qty > 0); + +DO $$ BEGIN + ALTER TABLE public.inventory_template_items + ADD CONSTRAINT inventory_template_items_smart_group_matches_mode + CHECK ((par_mode = 'smart') = (smart_group IS NOT NULL)); +EXCEPTION WHEN duplicate_object THEN NULL; END $$; + +-- ── Property-level items ──────────────────────────────────────────────────── +-- par_level (existing) becomes the resolved CACHE when par_mode = 'smart'. +-- auto_adjust=false lets a PM keep smart-formula behavior but pin the item +-- against historical overrides. par_resolved_at is observability only. + +ALTER TABLE public.inventory_items + ADD COLUMN IF NOT EXISTS par_mode par_mode NOT NULL DEFAULT 'static', + ADD COLUMN IF NOT EXISTS smart_group par_smart_group, + ADD COLUMN IF NOT EXISTS base_qty numeric NOT NULL DEFAULT 1 CHECK (base_qty > 0), + ADD COLUMN IF NOT EXISTS auto_adjust boolean NOT NULL DEFAULT true, + ADD COLUMN IF NOT EXISTS par_resolved_at timestamptz; + +DO $$ BEGIN + ALTER TABLE public.inventory_items + ADD CONSTRAINT inventory_items_smart_group_matches_mode + CHECK ((par_mode = 'smart') = (smart_group IS NOT NULL)); +EXCEPTION WHEN duplicate_object THEN NULL; END $$; + +-- ── Consumption stats (populated by pass 2's Inngest work) ────────────────── +-- One rolling aggregate per (property, item). Rows are written exclusively by +-- the service role inside Inngest steps; org members get read-only access so +-- the par-levels UI can explain WHY a smart par is what it is. + +CREATE TABLE IF NOT EXISTS public.inventory_consumption_stats ( + property_id uuid NOT NULL REFERENCES public.properties(id) ON DELETE CASCADE, + inventory_item_id uuid NOT NULL REFERENCES public.inventory_items(id) ON DELETE CASCADE, + org_id uuid NOT NULL REFERENCES public.organizations(id) ON DELETE CASCADE, + avg_rate_per_guest_night numeric NOT NULL DEFAULT 0 CHECK (avg_rate_per_guest_night >= 0), + sample_count integer NOT NULL DEFAULT 0 CHECK (sample_count >= 0), + last_sample_at timestamptz, + updated_at timestamptz NOT NULL DEFAULT now(), + PRIMARY KEY (property_id, inventory_item_id) +); + +CREATE INDEX IF NOT EXISTS idx_inventory_consumption_stats_org_id + ON public.inventory_consumption_stats (org_id); + +-- inventory_item_id is an FK but only the SECOND column of the primary key, so +-- the PK's index does not cover it — a lookup or an ON DELETE CASCADE from +-- inventory_items would seq-scan. scripts/check-db-invariants.mjs fails on any +-- FK column without a covering index; property_id is covered as the PK's +-- leading column and org_id by the index above, this one was the gap. +CREATE INDEX IF NOT EXISTS idx_inventory_consumption_stats_item_id + ON public.inventory_consumption_stats (inventory_item_id); + +CREATE OR REPLACE TRIGGER inventory_consumption_stats_updated_at + BEFORE UPDATE ON public.inventory_consumption_stats + FOR EACH ROW EXECUTE FUNCTION public.set_updated_at(); + +ALTER TABLE public.inventory_consumption_stats ENABLE ROW LEVEL SECURITY; + +GRANT SELECT ON TABLE public.inventory_consumption_stats TO authenticated; +GRANT SELECT, INSERT, UPDATE, DELETE ON TABLE public.inventory_consumption_stats TO service_role; + +-- Read-only for org members. No INSERT/UPDATE/DELETE policies for +-- authenticated at all — writes go through service-role Inngest steps only. +-- get_user_org_ids(), not a hand-rolled organization_members subquery: the +-- helper is the one place the membership rule lives (it also requires +-- invite_accepted_at IS NOT NULL, which the raw subquery this replaced did +-- not, so that version would have shown stats to members with a pending +-- invite). CLAUDE.md → Critical Security Rules #2. +DROP POLICY IF EXISTS "inventory_consumption_stats_select" ON public.inventory_consumption_stats; +CREATE POLICY "inventory_consumption_stats_select" + ON public.inventory_consumption_stats FOR SELECT + USING (org_id IN (SELECT get_user_org_ids())); diff --git a/supabase/migrations/20260811020000_fix_par_stats_junction_ambiguity.sql b/supabase/migrations/20260811020000_fix_par_stats_junction_ambiguity.sql new file mode 100644 index 00000000..d8b97a70 --- /dev/null +++ b/supabase/migrations/20260811020000_fix_par_stats_junction_ambiguity.sql @@ -0,0 +1,114 @@ +-- Fix: 20260810214329_dynamic_par_engine_schema.sql broke every PostgREST +-- embed between inventory_items and properties. +-- +-- inventory_consumption_stats was created with PRIMARY KEY (property_id, +-- inventory_item_id), where both columns are single-column foreign keys to +-- different tables. That is exactly PostgREST's signature for a many-to-many +-- JUNCTION table, so it began offering a second path between inventory_items +-- and properties on top of the existing inventory_items.property_id FK. Every +-- pre-existing embed became ambiguous and started returning +-- HTTP 300 / PGRST201 "Could not embed because more than one relationship was +-- found" — four live call sites: the inventory page, inventory/actions.ts, +-- lib/notifications.ts (the low-stock notification bell) and +-- lib/support/account-tools.ts. CI caught only the first, via +-- e2e/specs/07-inventory.spec.ts; the other three were broken in production +-- with nothing failing. +-- +-- Verified empirically against a throwaway three-table fixture on the E2E +-- project rather than reasoned from the docs: a composite PK of exactly two +-- single-column FKs produces PGRST201, and replacing it with ANY other primary +-- key removes the detection. A UNIQUE constraint on the same pair does NOT +-- trigger it — the detection keys on the PRIMARY KEY specifically. +-- +-- The fix is to drop property_id rather than to bolt on a surrogate PK: +-- inventory_items is already a property-level table, so +-- inventory_consumption_stats.property_id was derivable from +-- inventory_item_id and could drift out of agreement with it. Dropping it +-- makes the true grain — one rolling aggregate per property-level inventory +-- item — the primary key, and a single-column PK can never be read as a +-- junction, so this cannot regress on this table. +-- +-- org_id STAYS. It is equally derivable but is load-bearing for the RLS +-- policy, which is a deliberate denormalization rather than an accident. +-- +-- Both projects held 0 rows when this ran, so no data migration is needed. + +-- ── Rebuild the key ───────────────────────────────────────────────────────── + +ALTER TABLE public.inventory_consumption_stats + DROP CONSTRAINT IF EXISTS inventory_consumption_stats_pkey; + +-- Drops the column, its FK to properties, and idx_inventory_consumption_stats_ +-- item_id's reason to exist in one step. +ALTER TABLE public.inventory_consumption_stats + DROP COLUMN IF EXISTS property_id; + +DO $$ BEGIN + ALTER TABLE public.inventory_consumption_stats + ADD CONSTRAINT inventory_consumption_stats_pkey + PRIMARY KEY (inventory_item_id); +EXCEPTION WHEN invalid_table_definition THEN NULL; END $$; + +-- inventory_item_id is now the PK's only column, so the PK index covers that +-- FK and this separate index is a duplicate. (org_id keeps its own index — +-- scripts/check-db-invariants.mjs requires a covering index on every FK +-- column, and org_id is still an FK.) +DROP INDEX IF EXISTS public.idx_inventory_consumption_stats_item_id; + +-- ── Guardrail ─────────────────────────────────────────────────────────────── +-- Structural backstop so the next table with this shape fails CI instead of +-- silently breaking unrelated embeds. Returns one row per public table whose +-- PRIMARY KEY is exactly two columns, each a single-column FK to a different +-- table. scripts/check-db-invariants.mjs fails on any row it returns. +-- +-- SECURITY INVOKER (the default) and read-only over pg_catalog: it exposes no +-- row data, only relationship shape, and the CI job already connects with the +-- service role. +CREATE OR REPLACE FUNCTION public.accidental_junction_tables() +RETURNS TABLE (junction_table text, pk_columns text[], parents text[]) +LANGUAGE sql +STABLE +AS $$ + WITH pk AS ( + SELECT c.conrelid AS tbl, + array_agg(a.attname ORDER BY a.attname) AS cols, + count(*) AS n + FROM pg_constraint c + JOIN unnest(c.conkey) k(attnum) ON true + JOIN pg_attribute a ON a.attrelid = c.conrelid AND a.attnum = k.attnum + WHERE c.contype = 'p' + AND c.connamespace = 'public'::regnamespace + GROUP BY c.conrelid + ), + fk AS ( + SELECT c.conrelid AS tbl, + a.attname AS col, + c.confrelid::regclass::text AS parent + FROM pg_constraint c + JOIN unnest(c.conkey) k(attnum) ON true + JOIN pg_attribute a ON a.attrelid = c.conrelid AND a.attnum = k.attnum + WHERE c.contype = 'f' + AND array_length(c.conkey, 1) = 1 + ) + SELECT pk.tbl::regclass::text, + pk.cols, + array_agg(DISTINCT fk.parent) + FROM pk + JOIN fk ON fk.tbl = pk.tbl AND fk.col = ANY (pk.cols) + WHERE pk.n = 2 + GROUP BY pk.tbl, pk.cols + HAVING count(DISTINCT fk.col) = 2 + AND count(DISTINCT fk.parent) = 2; +$$; + +COMMENT ON FUNCTION public.accidental_junction_tables() IS + 'Tables PostgREST will read as many-to-many junctions (PK = exactly two ' + 'single-column FKs to two different tables), making both parents'' embeds ' + 'ambiguous with PGRST201. A genuine join table belongs in the allowlist in ' + 'scripts/check-db-invariants.mjs; anything else is the 20260810214329 bug.'; + +GRANT EXECUTE ON FUNCTION public.accidental_junction_tables() TO service_role; + +-- PostgREST caches the schema; without this the ambiguity persists until the +-- next reload even though the FK is gone. +NOTIFY pgrst, 'reload schema'; diff --git a/types/database.generated.ts b/types/database.generated.ts index 8009f3c5..7a780cb6 100644 --- a/types/database.generated.ts +++ b/types/database.generated.ts @@ -1748,6 +1748,7 @@ export type Database = { } inventory_catalog: { Row: { + base_qty: number category: Database["public"]["Enums"]["inventory_category"] created_at: string default_par_level: number @@ -1756,8 +1757,11 @@ export type Database = { id: string is_active: boolean name: string + par_mode: Database["public"]["Enums"]["par_mode"] + smart_group: Database["public"]["Enums"]["par_smart_group"] | null } Insert: { + base_qty?: number category?: Database["public"]["Enums"]["inventory_category"] created_at?: string default_par_level?: number @@ -1766,8 +1770,11 @@ export type Database = { id?: string is_active?: boolean name: string + par_mode?: Database["public"]["Enums"]["par_mode"] + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null } Update: { + base_qty?: number category?: Database["public"]["Enums"]["inventory_category"] created_at?: string default_par_level?: number @@ -1776,6 +1783,8 @@ export type Database = { id?: string is_active?: boolean name?: string + par_mode?: Database["public"]["Enums"]["par_mode"] + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null } Relationships: [] } @@ -1818,6 +1827,48 @@ export type Database = { }, ] } + inventory_consumption_stats: { + Row: { + avg_rate_per_guest_night: number + inventory_item_id: string + last_sample_at: string | null + org_id: string + sample_count: number + updated_at: string + } + Insert: { + avg_rate_per_guest_night?: number + inventory_item_id: string + last_sample_at?: string | null + org_id: string + sample_count?: number + updated_at?: string + } + Update: { + avg_rate_per_guest_night?: number + inventory_item_id?: string + last_sample_at?: string | null + org_id?: string + sample_count?: number + updated_at?: string + } + Relationships: [ + { + foreignKeyName: "inventory_consumption_stats_inventory_item_id_fkey" + columns: ["inventory_item_id"] + isOneToOne: false + referencedRelation: "inventory_items" + referencedColumns: ["id"] + }, + { + foreignKeyName: "inventory_consumption_stats_org_id_fkey" + columns: ["org_id"] + isOneToOne: false + referencedRelation: "organizations" + referencedColumns: ["id"] + }, + ] + } inventory_counts: { Row: { created_at: string @@ -1872,6 +1923,8 @@ export type Database = { } inventory_items: { Row: { + auto_adjust: boolean + base_qty: number catalog_item_id: string | null category: Database["public"]["Enums"]["inventory_category"] created_at: string @@ -1884,13 +1937,18 @@ export type Database = { notes: string | null org_id: string par_level: number + par_mode: Database["public"]["Enums"]["par_mode"] + par_resolved_at: string | null preferred_brand: string | null property_id: string + smart_group: Database["public"]["Enums"]["par_smart_group"] | null source_template_id: string | null unit: string updated_at: string } Insert: { + auto_adjust?: boolean + base_qty?: number catalog_item_id?: string | null category?: Database["public"]["Enums"]["inventory_category"] created_at?: string @@ -1903,13 +1961,18 @@ export type Database = { notes?: string | null org_id: string par_level?: number + par_mode?: Database["public"]["Enums"]["par_mode"] + par_resolved_at?: string | null preferred_brand?: string | null property_id: string + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null source_template_id?: string | null unit?: string updated_at?: string } Update: { + auto_adjust?: boolean + base_qty?: number catalog_item_id?: string | null category?: Database["public"]["Enums"]["inventory_category"] created_at?: string @@ -1922,8 +1985,11 @@ export type Database = { notes?: string | null org_id?: string par_level?: number + par_mode?: Database["public"]["Enums"]["par_mode"] + par_resolved_at?: string | null preferred_brand?: string | null property_id?: string + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null source_template_id?: string | null unit?: string updated_at?: string @@ -1961,40 +2027,49 @@ export type Database = { } inventory_template_items: { Row: { + base_qty: number catalog_item_id: string | null category: string | null id: string name: string notes: string | null par_level: number + par_mode: Database["public"]["Enums"]["par_mode"] par_qty: number preferred_brand: string | null + smart_group: Database["public"]["Enums"]["par_smart_group"] | null sort_order: number template_id: string unit: string | null } Insert: { + base_qty?: number catalog_item_id?: string | null category?: string | null id?: string name: string notes?: string | null par_level?: number + par_mode?: Database["public"]["Enums"]["par_mode"] par_qty?: number preferred_brand?: string | null + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null sort_order?: number template_id: string unit?: string | null } Update: { + base_qty?: number catalog_item_id?: string | null category?: string | null id?: string name?: string notes?: string | null par_level?: number + par_mode?: Database["public"]["Enums"]["par_mode"] par_qty?: number preferred_brand?: string | null + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null sort_order?: number template_id?: string unit?: string | null @@ -2556,6 +2631,7 @@ export type Database = { } org_inventory_catalog: { Row: { + base_qty: number category: Database["public"]["Enums"]["inventory_category"] created_at: string default_par_level: number @@ -2565,10 +2641,13 @@ export type Database = { is_active: boolean name: string org_id: string + par_mode: Database["public"]["Enums"]["par_mode"] platform_catalog_item_id: string | null + smart_group: Database["public"]["Enums"]["par_smart_group"] | null updated_at: string } Insert: { + base_qty?: number category?: Database["public"]["Enums"]["inventory_category"] created_at?: string default_par_level?: number @@ -2578,10 +2657,13 @@ export type Database = { is_active?: boolean name: string org_id: string + par_mode?: Database["public"]["Enums"]["par_mode"] platform_catalog_item_id?: string | null + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null updated_at?: string } Update: { + base_qty?: number category?: Database["public"]["Enums"]["inventory_category"] created_at?: string default_par_level?: number @@ -2591,7 +2673,9 @@ export type Database = { is_active?: boolean name?: string org_id?: string + par_mode?: Database["public"]["Enums"]["par_mode"] platform_catalog_item_id?: string | null + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null updated_at?: string } Relationships: [ @@ -3179,30 +3263,39 @@ export type Database = { } platform_inventory_template_items: { Row: { + base_qty: number catalog_item_id: string created_at: string id: string par_level: number + par_mode: Database["public"]["Enums"]["par_mode"] platform_inventory_template_id: string preferred_brand: string | null + smart_group: Database["public"]["Enums"]["par_smart_group"] | null sort_order: number } Insert: { + base_qty?: number catalog_item_id: string created_at?: string id?: string par_level?: number + par_mode?: Database["public"]["Enums"]["par_mode"] platform_inventory_template_id: string preferred_brand?: string | null + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null sort_order?: number } Update: { + base_qty?: number catalog_item_id?: string created_at?: string id?: string par_level?: number + par_mode?: Database["public"]["Enums"]["par_mode"] platform_inventory_template_id?: string preferred_brand?: string | null + smart_group?: Database["public"]["Enums"]["par_smart_group"] | null sort_order?: number } Relationships: [ @@ -6000,6 +6093,11 @@ export type Database = { | "past_due" | "cancelled" | "paused" + par_mode: "static" | "smart" + par_smart_group: + | "bathroom_essential" + | "bedroom_essential" + | "guest_consumable" po_status: | "draft" | "sent" @@ -6308,6 +6406,12 @@ export const Constants = { "cancelled", "paused", ], + par_mode: ["static", "smart"], + par_smart_group: [ + "bathroom_essential", + "bedroom_essential", + "guest_consumable", + ], po_status: [ "draft", "sent", diff --git a/types/database.ts b/types/database.ts index b492b579..0eaed8c1 100644 --- a/types/database.ts +++ b/types/database.ts @@ -30,6 +30,14 @@ export type PriorityLevel = 'low' | 'medium' | 'high' | 'urgent' export type ContactPref = 'email' | 'sms' | 'both' export type ChecklistStatus = 'not_started' | 'in_progress' | 'completed' export type InventoryCategory = 'paper_goods' | 'cleaning' | 'kitchen' | 'bath' | 'laundry' | 'bedroom' | 'bedroom_linens' | 'outdoor' | 'maintenance_safety' | 'guest_experience' | 'technology' | 'other' +// Declared HERE, not re-exported from lib/inventory/par-engine.ts, even though +// that module is the par engine's source of truth for everything else. +// scripts/check-type-drift.mjs diffs each Postgres enum against a TS union it +// parses out of THIS file with /^export type (\w+)\s*=/ — a brace re-export +// (`export type { ParMode }`) matches nothing, so the gate would report a +// parse miss rather than a real comparison. par-engine.ts imports these back. +export type ParMode = 'static' | 'smart' +export type ParSmartGroup = 'bathroom_essential' | 'bedroom_essential' | 'guest_consumable' export type PoStatus = 'draft' | 'sent' | 'acknowledged' | 'ordered' | 'received' | 'cancelled' export type VendorSpecialty = 'plumbing' | 'electrical' | 'hvac' | 'landscaping' | 'cleaning' | 'pest_control' | 'pool' | 'roofing' | 'general' | 'other' export type WoStatus = 'pending' | 'quote_requested' | 'assigned' | 'in_progress' | 'completed' | 'cancelled' @@ -445,6 +453,9 @@ export interface OrgInventoryCatalogItem { category: InventoryCategory default_unit: string default_par_level: number + par_mode: ParMode + smart_group: ParSmartGroup | null + base_qty: number description: string | null is_active: boolean created_at: string @@ -633,11 +644,37 @@ export interface InventoryCatalogItem { category: InventoryCategory default_unit: string default_par_level: number + par_mode: ParMode + smart_group: ParSmartGroup | null + base_qty: number description: string | null is_active: boolean created_at: string } +/** + * Rolling consumption aggregate, one row per (property, item). + * + * Service-role write only — populated by the dynamic PAR engine's Inngest + * steps; org members hold a SELECT policy so the par-levels UI can explain + * why a smart par resolved the way it did. No primary `id`: the PK is the + * composite (property_id, inventory_item_id). + */ +// PK is inventory_item_id alone. There is deliberately NO property_id: an +// inventory_item is already property-level, so the column was derivable, and +// a PK of (property_id, inventory_item_id) made PostgREST read this table as +// a many-to-many junction between properties and inventory_items — which broke +// every pre-existing embed between them with PGRST201. See +// 20260811020000_fix_par_stats_junction_ambiguity.sql. +export interface InventoryConsumptionStats { + inventory_item_id: string + org_id: string + avg_rate_per_guest_night: number + sample_count: number + last_sample_at: string | null + updated_at: string +} + export interface InventoryItem { id: string property_id: string @@ -648,6 +685,11 @@ export interface InventoryItem { category: InventoryCategory unit: string par_level: number + par_mode: ParMode + smart_group: ParSmartGroup | null + base_qty: number + auto_adjust: boolean + par_resolved_at: string | null current_quantity: number low_stock_threshold_pct: number is_active: boolean @@ -1213,6 +1255,9 @@ export interface PlatformInventoryTemplateItem { platform_inventory_template_id: string catalog_item_id: string par_level: number + par_mode: ParMode + smart_group: ParSmartGroup | null + base_qty: number preferred_brand: string | null sort_order: number created_at: string @@ -1231,6 +1276,9 @@ export interface InventoryTemplateItem { category: InventoryCategory | null unit: string | null par_level: number + par_mode: ParMode + smart_group: ParSmartGroup | null + base_qty: number // Legacy column from the original 20260604223335_add_inventory_templates.sql // schema, superseded by par_level (added later) — never read or written // by current app code (see actions.ts's "par_qty (unused, see Pass 1/3 @@ -1833,6 +1881,7 @@ export interface HandWrittenRowMap { checklist_instance_items: ChecklistInstanceItem inventory_catalog: InventoryCatalogItem inventory_items: InventoryItem + inventory_consumption_stats: InventoryConsumptionStats inventory_counts: InventoryCount inventory_count_items: InventoryCountItem purchase_orders: PurchaseOrder diff --git a/unit/inventory/par-engine.test.ts b/unit/inventory/par-engine.test.ts new file mode 100644 index 00000000..3b304781 --- /dev/null +++ b/unit/inventory/par-engine.test.ts @@ -0,0 +1,126 @@ +import { describe, it, expect } from 'vitest' +import { + resolvePar, + normalizeParConfig, + HISTORICAL_FLOOR, + type ParItemConfig, + type ParPropertyContext, + type ParConsumptionStats, +} from '@/lib/inventory/par-engine' + +const baseProperty: ParPropertyContext = { + bathrooms: 2, + bedrooms: 3, + max_guests: 6, + avg_stay_length: 3, +} + +const baseConfig: ParItemConfig = { + par_mode: 'static', + smart_group: null, + base_qty: 1, + par_level: 5, + auto_adjust: true, +} + +describe('resolvePar', () => { + it('static mode returns stored par_level untouched even with qualifying stats', () => { + const stats: ParConsumptionStats = { avg_rate_per_guest_night: 0.5, sample_count: 3 } + const result = resolvePar(baseConfig, baseProperty, stats) + expect(result).toEqual({ par: 5, source: 'static' }) + }) + + it('bathroom_essential formula: ceil(2 × 2.5 × 1.15) = 6', () => { + const config: ParItemConfig = { ...baseConfig, par_mode: 'smart', smart_group: 'bathroom_essential', base_qty: 2 } + const property: ParPropertyContext = { ...baseProperty, bathrooms: 2.5 } + const result = resolvePar(config, property, null) + expect(result).toEqual({ par: 6, source: 'smart_formula' }) + }) + + it('bedroom_essential formula: ceil(4 × 3 × 1.20) = 15', () => { + const config: ParItemConfig = { ...baseConfig, par_mode: 'smart', smart_group: 'bedroom_essential', base_qty: 4 } + const property: ParPropertyContext = { ...baseProperty, bedrooms: 3 } + const result = resolvePar(config, property, null) + expect(result).toEqual({ par: 15, source: 'smart_formula' }) + }) + + it('guest_consumable formula: ceil(1 × 6 × 1.10) = 7', () => { + const config: ParItemConfig = { ...baseConfig, par_mode: 'smart', smart_group: 'guest_consumable', base_qty: 1 } + const property: ParPropertyContext = { ...baseProperty, max_guests: 6 } + const result = resolvePar(config, property, null) + expect(result).toEqual({ par: 7, source: 'smart_formula' }) + }) + + it('null bathrooms degrades to multiplier 1, never 0 or NaN', () => { + const config: ParItemConfig = { ...baseConfig, par_mode: 'smart', smart_group: 'bathroom_essential', base_qty: 2 } + const property: ParPropertyContext = { ...baseProperty, bathrooms: null } + const result = resolvePar(config, property, null) + expect(result.par).toBe(Math.ceil(2 * 1 * 1.15)) + expect(Number.isNaN(result.par)).toBe(false) + expect(result.source).toBe('smart_formula') + }) + + it('historical: rate 0.5, sample_count 3, max_guests 6, avg_stay_length 3 → ceil(0.5 × 6 × 3 × 1.20) = 11', () => { + const config: ParItemConfig = { ...baseConfig, par_mode: 'smart', smart_group: 'guest_consumable', base_qty: 1 } + const stats: ParConsumptionStats = { avg_rate_per_guest_night: 0.5, sample_count: 3 } + const result = resolvePar(config, baseProperty, stats) + expect(result).toEqual({ par: 11, source: 'historical' }) + }) + + it('historical floor: a tiny rate resolves to HISTORICAL_FLOOR', () => { + const config: ParItemConfig = { ...baseConfig, par_mode: 'smart', smart_group: 'guest_consumable', base_qty: 1 } + const stats: ParConsumptionStats = { avg_rate_per_guest_night: 0.001, sample_count: 3 } + const result = resolvePar(config, baseProperty, stats) + expect(result).toEqual({ par: HISTORICAL_FLOOR, source: 'historical' }) + }) + + it('sample_count below threshold falls back to smart_formula', () => { + const config: ParItemConfig = { ...baseConfig, par_mode: 'smart', smart_group: 'guest_consumable', base_qty: 1 } + const stats: ParConsumptionStats = { avg_rate_per_guest_night: 0.5, sample_count: 2 } + const result = resolvePar(config, baseProperty, stats) + expect(result.source).toBe('smart_formula') + }) + + it('auto_adjust: false with qualifying stats falls back to smart_formula (the pin works)', () => { + const config: ParItemConfig = { + ...baseConfig, + par_mode: 'smart', + smart_group: 'guest_consumable', + base_qty: 1, + auto_adjust: false, + } + const stats: ParConsumptionStats = { avg_rate_per_guest_night: 0.5, sample_count: 3 } + const result = resolvePar(config, baseProperty, stats) + expect(result.source).toBe('smart_formula') + }) + + it('smart mode with smart_group null (defensive) returns >= 1 and does not throw', () => { + const config: ParItemConfig = { ...baseConfig, par_mode: 'smart', smart_group: null, par_level: 3 } + expect(() => resolvePar(config, baseProperty, null)).not.toThrow() + const result = resolvePar(config, baseProperty, null) + expect(result.par).toBeGreaterThanOrEqual(1) + }) +}) + +describe('normalizeParConfig', () => { + it('passes through smart mode with a valid group', () => { + const result = normalizeParConfig({ par_mode: 'smart', smart_group: 'guest_consumable', base_qty: 2 }) + expect(result).toEqual({ par_mode: 'smart', smart_group: 'guest_consumable', base_qty: 2 }) + }) + + it('degrades smart with a null group to static', () => { + const result = normalizeParConfig({ par_mode: 'smart', smart_group: null, base_qty: 2 }) + expect(result).toEqual({ par_mode: 'static', smart_group: null, base_qty: 2 }) + }) + + it('strips the group when static (even if one was submitted)', () => { + const result = normalizeParConfig({ par_mode: 'static', smart_group: 'bedroom_essential', base_qty: 3 }) + expect(result).toEqual({ par_mode: 'static', smart_group: null, base_qty: 3 }) + }) + + it('clamps NaN/0/-3 base_qty to 1', () => { + expect(normalizeParConfig({ par_mode: 'static', smart_group: null, base_qty: NaN }).base_qty).toBe(1) + expect(normalizeParConfig({ par_mode: 'static', smart_group: null, base_qty: 0 }).base_qty).toBe(1) + expect(normalizeParConfig({ par_mode: 'static', smart_group: null, base_qty: -3 }).base_qty).toBe(1) + }) +})