Claude/hostile code audit rbz3f8 - #611
Conversation
First piece of the dynamic PAR engine, ported from
claude/migration-reconciliation-5is9pq (PAR_1 of 3, authored 2026-07-30).
Pure TypeScript with no database or network dependency, so it stands alone
and is fully covered by unit tests.
resolvePar() decides an inventory item's par level from:
- static mode — the stored par_level, untouched
- historical — observed consumption per guest-night, once an item has
>= 3 samples AND auto_adjust is on
- smart formula — base_qty x the property's bathrooms/bedrooms/max_guests,
plus a per-group buffer
Total by construction: a malformed row (smart mode with no group), a null
bathrooms, or a property with no metadata all degrade to a sensible value
rather than throwing inside an Inngest step or writing a par of 0.
NOTHING IMPORTS THIS YET, deliberately. The rest of the feature — the schema
that stores the config, the recompute pipeline, and the admin/PM UI — cannot
land until its migrations are applied, because lib/supabase/server.ts wires
createServerClient<Database> and types/database.generated.ts is generated
from the live schema. Declaring the new columns in types/database.ts before
the database has them breaks `.select('*')` inference (verified: it fails
app/(dashboard)/inventory/page.tsx's InventoryItemRow). Committing the
migrations ahead of applying them is equally not an option —
check-migration-ledger.mjs counts a local file with no ledger row as a
parity break, and a new migration is by definition outside the frozen
baseline.
So this commit is the part with no such dependency. The three migrations are
written and corrected but held back pending a go-ahead to apply; the
corrections are recorded here so they are not lost:
- renumbered to 20260810120000/130000/140000. The original
20260730140000 collided with main's already-applied
20260730140000_atomic_subscription_plan_update.sql, which would have
made `supabase db push` skip the PAR RPC silently.
- the consumption-stats RLS policy uses get_user_org_ids() rather than the
spec's hand-rolled organization_members subquery. Verified against the
live function definition: it also requires invite_accepted_at IS NOT
NULL, so the specced version would have shown stats to members with a
pending invite.
- added an index on inventory_consumption_stats.inventory_item_id. It is
an FK but only the SECOND column of the composite primary key, so the
PK's index does not cover it — check-db-invariants.mjs fails on that,
and an ON DELETE CASCADE from inventory_items would seq-scan.
Verified: tsc, 14 resolver tests, lint 181/181.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
…projects
Pass 1 of the dynamic PAR engine's schema, applied to production
(vpmznjktllhmmbfnxuvk) and E2E (syhthijeqlnltufdawyb) and committed at the
version the ledger actually recorded.
Adds par_mode/smart_group/base_qty down the whole catalog -> template ->
item chain (inventory_catalog, org_inventory_catalog,
platform_inventory_template_items, inventory_template_items,
inventory_items), plus auto_adjust/par_resolved_at on inventory_items and the
inventory_consumption_stats table the historical engine reads.
NO BEHAVIOUR CHANGES. Every column defaults to par_mode 'static', which is
what every existing row already behaves as, and nothing reads the new columns
yet — verified post-apply: 0 of 147 catalog rows are non-static.
── The ledger/file parity problem, and how it was handled ──────────────────
This had to go through the Supabase MCP: there is no CLI and no
SUPABASE_ACCESS_TOKEN in this environment, which is exactly the case
CLAUDE.md anticipates ("sometimes the only option"). MCP apply_migration
picks its OWN version rather than taking one from a filename, and it picks a
fresh one per call — so the same migration landed as:
E2E 20260810214329
prod 20260810214410
file 20260810120000 (the version I had chosen)
Three different versions for one migration. Left alone that is a local-only
file AND two ledger-only rows — the precise drift that put production at
36/35 divergences in audit H10, and check-migration-ledger.mjs fails on it in
both directions.
Reconciled to a single version, 20260810214329 (E2E's, the first recorded):
prod's ledger row was updated to match, and the local file renamed to it.
All three now agree. Verified against production after the fact: par_mode on
5 tables, inventory_consumption_stats present with RLS enabled, 1 policy, 3
indexes.
── Three corrections to the migration as specced ───────────────────────────
- RLS policy uses get_user_org_ids() instead of a hand-rolled
organization_members subquery. Checked the live function: it also requires
invite_accepted_at IS NOT NULL, so the specced version would have exposed
consumption stats to members with an unaccepted invite.
- Added an index on inventory_consumption_stats.inventory_item_id. It is an
FK but only the SECOND column of the composite PK, so the PK's index does
not cover it — check-db-invariants.mjs fails on that, and an ON DELETE
CASCADE from inventory_items would seq-scan.
- Made every ADD CONSTRAINT and the CREATE POLICY idempotent (duplicate_object
DO blocks, DROP POLICY IF EXISTS). CLAUDE.md requires all DDL to be
re-runnable; ADD CONSTRAINT has no IF NOT EXISTS, so the file as written
could only ever be applied once.
Also files FUTURE_REMEDIATION 32, the agreed algorithm follow-up: replace the
flat 20% buffer with variance-based safety stock (Welford + z-score), use an
EWMA so recent counts dominate, and size par to a restock CYCLE — 3-4 stays
at a 2.5-day lead time — rather than the single stay it assumes today.
types/database.ts is deliberately NOT updated here: nothing selects the new
columns yet, and adding them before types/database.generated.ts is
regenerated breaks `.select('*')` inference. Both land with Pass 2.
Verified: tsc, lint 181/181, tree parity-clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
docs/PAR_ENGINE_PORT_STATE.md — where the dynamic PAR engine port actually stands, what is applied vs parked, the four defects that must be fixed on the way through, and the design decisions this session surfaced that are not written anywhere else. The parts that would otherwise be lost: - The owner's par methodology, which was never on the seed sheet and is not uniform. Towels are per guest per stay with NO multi-stay coverage because they are laundered; sheets and pillows are per bed plus a spare; the welcome pack is per stay regardless of guests; toilet paper and K-cups were sized for 3 stays. The sheet's numbers decode against this exactly — toilet paper 18 = 1/guest x 6 guests x 3 stays. - The two design gaps that follow from it. The engine models what an item scales BY, but not whether it is CONSUMED or REUSED, and has no per-stay dimension at all. Without a consumable flag, FUTURE_REMEDIATION 32's coverage multiplier would take bath towels from 14 to 49. - The MCP ledger trap, in operational detail. MCP apply_migration assigns its own version, different per call: this migration landed as 20260810214329 on E2E and 20260810214410 on prod against a local file saying 20260810120000. Reconciled by hand; the next two migrations need the same treatment, and the doc gives the sequence. - The type-generation ordering that blocks the port (apply -> regenerate -> types/database.ts -> compiles), and why types/database.ts was deliberately left alone in d584a2a. Docs-only; nothing here changes behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds the Dynamic PAR Engine schema, a synchronous resolution engine, configuration normalization, type models, tests, migration safeguards, port-state documentation, and global Supabase error-handling checks. ChangesDynamic PAR engine
Supabase repository guardrails
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant InventoryItem
participant ParEngine
participant ConsumptionStats
InventoryItem->>ParEngine: configuration and property context
ConsumptionStats->>ParEngine: consumption aggregates
ParEngine->>InventoryItem: resolved PAR value and source
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@FUTURE_REMEDIATION.md`:
- Around line 1513-1536: Clarify the documentation before prescribing the
restock formula by defining projected guest-nights for the selected 3–4-stay
restock cycle. Express mean demand and variance over that same cycle horizon,
and explicitly state whether M2 tracks per-guest-night, per-stay, or per-cycle
demand; ensure the safety-stock calculation uses consistent demand units
throughout.
In `@supabase/migrations/20260810214329_dynamic_par_engine_schema.sql`:
- Around line 139-154: Update the RLS policy definitions for
inventory_consumption_stats after ENABLE ROW LEVEL SECURITY to add INSERT,
UPDATE, and DELETE policies alongside inventory_consumption_stats_select. Scope
each write policy with get_user_org_ids() and is_org_member(), using both USING
and WITH CHECK clauses as required; preserve the existing service_role grants
and ingestion behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8caff28f-876f-410e-8739-02241a61a741
📒 Files selected for processing (5)
FUTURE_REMEDIATION.mddocs/PAR_ENGINE_PORT_STATE.mdlib/inventory/par-engine.tssupabase/migrations/20260810214329_dynamic_par_engine_schema.sqlunit/inventory/par-engine.test.ts
| `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. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Define demand units before prescribing the restock formula.
avg_rate_per_guest_night cannot be multiplied directly by lead_time in days. The result has incompatible units. The coverage target also uses stays, not days.
Define projected guest-nights for the selected 3–4-stay restock cycle. Then calculate mean demand and variance over that same horizon. Define whether M2 tracks per-guest-night, per-stay, or per-cycle demand before implementing safety stock.
Proposed documentation correction
- par = rate x lead_time + Z x sigma x sqrt(lead_time)
+ par = rate_per_guest_night × projected_guest_nights_per_restock_cycle
+ + safety_stock_for_that_same_cycle📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `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. | |
| `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_per_guest_night × projected_guest_nights_per_restock_cycle | |
| safety_stock_for_that_same_cycle`. | |
| 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. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@FUTURE_REMEDIATION.md` around lines 1513 - 1536, Clarify the documentation
before prescribing the restock formula by defining projected guest-nights for
the selected 3–4-stay restock cycle. Express mean demand and variance over that
same cycle horizon, and explicitly state whether M2 tracks per-guest-night,
per-stay, or per-cycle demand; ensure the safety-stock calculation uses
consistent demand units throughout.
| 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())); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Define all required RLS policies for the new table.
inventory_consumption_stats has only a SELECT policy. Add INSERT, UPDATE, and DELETE policies. Scope each policy with get_user_org_ids() and is_org_member(). Use both USING and WITH CHECK for write policies.
Retain service-role ingestion if required. Service-role access does not replace the required RLS policy set.
As per coding guidelines, “Every new table migration must enable RLS and define SELECT, INSERT, UPDATE, and DELETE policies using get_user_org_ids() and is_org_member(); use both USING and WITH CHECK for write policies.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260810214329_dynamic_par_engine_schema.sql` around
lines 139 - 154, Update the RLS policy definitions for
inventory_consumption_stats after ENABLE ROW LEVEL SECURITY to add INSERT,
UPDATE, and DELETE policies alongside inventory_consumption_stats_select. Scope
each write policy with get_user_org_ids() and is_org_member(), using both USING
and WITH CHECK clauses as required; preserve the existing service_role grants
and ingestion behavior.
Source: Coding guidelines
CI's db-invariants job failed with 8 findings after the schema landed. My call to defer types/database.ts in d584a2a was wrong, and wrong in a specific way worth naming: I reasoned that nothing selects the new columns yet, so the types could wait. But check-type-drift.mjs diffs types/database.ts against the LIVE SCHEMA, not against usage. The moment the migration applied, deferring stopped being safe — it just moved the failure from tsc to the drift gate. Fixes, one per finding class: - ENUM_MAP in scripts/check-type-drift.mjs gains par_mode -> ParMode and par_smart_group -> ParSmartGroup. - The five hand-written interfaces gain their columns: par_mode/smart_group/ base_qty on InventoryCatalogItem, OrgInventoryCatalogItem, InventoryTemplateItem and PlatformInventoryTemplateItem, plus auto_adjust and par_resolved_at on InventoryItem. - New InventoryConsumptionStats interface, wired into HandWrittenRowMap. The map is the thing the gate reads — adding the table to the generated file alone does NOT satisfy it, which I only found by reading parseTableMap() rather than assuming the generated types were enough. - types/database.generated.ts hand-edited: 51 column entries across the five tables' Row/Insert/Update shapes, the two enums in both the type union and the runtime Constants block, and the inventory_consumption_stats block. Hand-editing a generated file breaks its own rule and is a deliberate, bounded exception: regenerating needs the whole 6,411-line file, and the edit was verified against the live schema instead — all 7 columns with correct nullability and default-optionality, and all three FK constraint names match. It should be regenerated properly at the next opportunity. ── One structural change, not cosmetic ───────────────────────────────────── ParMode and ParSmartGroup now DECLARE in types/database.ts, and lib/inventory/par-engine.ts imports them back (type-only, so the engine stays runtime-dependency-free) and re-exports for its existing callers. The spec had the engine own them and types/database.ts re-export via `export type { ParMode, ParSmartGroup }`. That parses to nothing: parseUnionTypes() matches /^export type (\w+)\s*=/, so a brace re-export would have failed the gate with "parse miss" rather than comparing anything — a second, quieter failure hiding behind the first. Enum unions belong in types/database.ts because that is the file the gate reads. Verified: the gate self-disarms locally without E2E credentials, so its three parsers and its column comparison were re-implemented against the live schema directly — 0 failures across all six affected tables, and both enums' labels match. Plus tsc, 3631 tests, lint 181/181, ui-classes, semgrep chokepoints, next build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
-discarded-result, -read-without-error and -read-without-error-fan-in have all sat at 0 since the 2026-08-07 and 2026-08-08 burn-downs cleared 159 + 14 live sites. They stayed in ratchet.yml, which means they only ever gated on --baseline-commit: a finding that is not visible in the diff view -- a moved file, 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. No site was fixed here. This only collects the gate upgrade those burn-downs earned and never took. All three move at ERROR with no paths.exclude, which is correct rather than lax: no file legitimately owns "discard a PostgREST error", so there is no owner to name, and every exemption is already expressed as the handling construct itself (binding the result, destructuring error, unwrap.ts). Fire-checked before promoting, same protocol as -cross-tenant and -global-table. A scratch fixture carried one deliberate violation per rule AND a correct control for each; semgrep reported exactly the three violations and none of the three controls. The controls are the half that matters -- a rule that fires on everything also "fires", and a rule at zero because it is broken is indistinguishable from one at zero because the tree is clean. Fixture reverted, whole tree re-run at --error, exit 0. Baseline keys deleted in the same change, per the promotion rule. What remains in ratchet.yml is the unbounded-select ladder alone: in-list 11, org-scoped 65, single-parent 16. Also notes the consequence for the parked PAR port: its four discarded read/write results now fail CI outright instead of pushing a baseline number up, and there is no nosemgrep escape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
20260810214329 created inventory_consumption_stats with PRIMARY KEY (property_id, inventory_item_id). Both columns are single-column FKs to different tables, which is exactly PostgREST's signature for a many-to-many JUNCTION table. It began offering a second path between inventory_items and properties on top of the existing property_id FK, so every pre-existing embed between them started returning HTTP 300 / PGRST201. Adding a table broke queries that never mention it. Four live call sites: the inventory page, inventory/actions.ts, lib/notifications.ts (the low-stock notification bell) and lib/support/account-tools.ts. Only the first had a test, so CI reported one red e2e spec while the other three were broken in production with every other gate green. Verified rather than reasoned. Reproduced the exact PGRST201 against the E2E project, then established the detection rule on a throwaway three-table fixture: a composite PK of two single-column FKs produces the ambiguity, and replacing it with any other PK removes it. A UNIQUE constraint on the same pair does NOT trigger it -- the detection keys on the PRIMARY KEY specifically. That is what ruled out the surrogate-PK workaround in favour of the fix below. Dropped property_id instead of bolting on a surrogate key. inventory_items is already property-level, so the column was derivable from its sibling and could drift out of agreement with it; the PK is now inventory_item_id alone, which states the real grain and structurally cannot be re-read as a junction. org_id stays -- equally derivable, but load-bearing for RLS. Both projects held 0 rows, so no data migration. Re-tested both embed directions afterwards: 401 (anon has no grants, as expected) rather than 300. Guardrail, per the meta-rule that a convention ships with its enforcement: public.accidental_junction_tables() plus check 10 in check-db-invariants.mjs, allowlist empty. Canaried in both halves -- the detector was confirmed to report a deliberately junction-shaped pair of tables on E2E and to return empty once dropped, and the script was driven against a stub with and without a finding (exit 0 / exit 1, with the diagnosis and both remediation paths in the message). Ledger reconciled to 20260811020000 on both projects; MCP had again assigned two different versions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.semgrep/chokepoints.yml:
- Around line 398-410: Expand both Supabase result-destructure rule groups at
.semgrep/chokepoints.yml lines 398-410 and 428-432 to match let and var
declarations, count-only destructuring, and aliased count bindings in addition
to current const data forms. Add corresponding error-exclusion patterns for
every new declaration shape, and ensure the expanded patterns also cover
Promise.all fan-ins; preserve the existing exclusions for destructures
containing error.
In `@supabase/migrations/20260811020000_fix_par_stats_junction_ambiguity.sql`:
- Around line 46-50: Update the primary-key creation block around
inventory_consumption_stats_pkey to stop swallowing invalid_table_definition
errors. Let ADD CONSTRAINT fail when the existing primary key has an unexpected
shape, or explicitly inspect and validate the existing constraint before
proceeding; do not treat the failed creation as successful.
In `@types/database.ts`:
- Around line 655-668: Update the InventoryConsumptionStats table documentation
by removing the obsolete description that claims rows are keyed by (property,
item) and use a composite primary key. Retain the explanation that
inventory_item_id is the sole primary key and property_id is intentionally
absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2582a2fa-57a9-4366-9673-0cbfe7ddf142
⛔ Files ignored due to path filters (1)
types/database.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (11)
.semgrep/README.md.semgrep/baseline-counts.json.semgrep/chokepoints.yml.semgrep/ratchet.ymlCLAUDE.mddocs/PAR_ENGINE_PORT_STATE.mdlib/inventory/par-engine.tsscripts/check-db-invariants.mjsscripts/check-type-drift.mjssupabase/migrations/20260811020000_fix_par_stats_junction_ambiguity.sqltypes/database.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/inventory/par-engine.ts
| - 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) ...> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/supabase-error-handling-fixture.ts" <<'EOF'
async function verifyCoverage() {
let { data } = await supabase.from('items').select()
const { count } = await supabase.from('items').select('*', { count: 'exact' })
let [{ data: batchData }] = await Promise.all([
supabase.from('items').select(),
])
const [{ count: batchCount }] = await Promise.all([
supabase.from('items').select('*', { count: 'exact' }),
])
let { data: handledData, error: handledError } = await supabase.from('items').select()
const [{ count: handledCount, error: handledBatchError }] = await Promise.all([
supabase.from('items').select('*', { count: 'exact' }),
])
}
EOF
semgrep --config .semgrep/chokepoints.yml --json "$tmpdir" |
jq '[.results[] | select(
.check_id == "fieldstay-supabase-read-without-error" or
.check_id == "fieldstay-supabase-read-without-error-fan-in"
)] | length'
# Expect 4 findings after the rule expansion and zero findings for the handled controls.Repository: smj1860/fieldstay
Length of output: 605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant rules ---'
sed -n '360,455p' .semgrep/chokepoints.yml
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/supabase-error-handling-fixture.ts" <<'EOF'
async function verifyCoverage() {
let { data } = await supabase.from('items').select()
var { data: aliasedData } = await supabase.from('items').select()
const { count } = await supabase.from('items').select('*', { count: 'exact' })
let { count: aliasedCount } = await supabase.from('items').select('*', { count: 'exact' })
let [{ data: batchData }] = await Promise.all([
supabase.from('items').select(),
])
var [{ data: varBatchData }] = await Promise.all([
supabase.from('items').select(),
])
const [{ count: batchCount }] = await Promise.all([
supabase.from('items').select('*', { count: 'exact' }),
])
let [{ count: letBatchCount }] = await Promise.all([
supabase.from('items').select('*', { count: 'exact' }),
])
let { data: handledData, error: handledError } = await supabase.from('items').select()
const [{ count: handledCount, error: handledBatchError }] = await Promise.all([
supabase.from('items').select('*', { count: 'exact' }),
])
}
EOF
semgrep --config .semgrep/chokepoints.yml --json "$tmpdir" > "$tmpdir/results.json"
jq '[.results[] | {check_id, path, start: .start.line, lines: .extra.lines}]' "$tmpdir/results.json"Repository: smj1860/fieldstay
Length of output: 6459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/supabase-error-handling-fixture.ts" <<'EOF'
async function verifyCoverage() {
let { data } = await supabase.from('items').select()
var { data: aliasedData } = await supabase.from('items').select()
const { count } = await supabase.from('items').select('*', { count: 'exact' })
let { count: aliasedCount } = await supabase.from('items').select('*', { count: 'exact' })
let [{ data: batchData }] = await Promise.all([
supabase.from('items').select(),
])
var [{ data: varBatchData }] = await Promise.all([
supabase.from('items').select(),
])
const [{ count: batchCount }] = await Promise.all([
supabase.from('items').select('*', { count: 'exact' }),
])
let [{ count: letBatchCount }] = await Promise.all([
supabase.from('items').select('*', { count: 'exact' }),
])
let { data: handledData, error: handledError } = await supabase.from('items').select()
const [{ count: handledCount, error: handledBatchError }] = await Promise.all([
supabase.from('items').select('*', { count: 'exact' }),
])
}
EOF
printf '%s\n' '--- rules ---'
sed -n '360,455p' .semgrep/chokepoints.yml
printf '%s\n' '--- all findings ---'
semgrep --config .semgrep/chokepoints.yml --json "$tmpdir" |
jq '[.results[] | {check_id, path, start_line: .start.line, matched: .extra.lines}]'Repository: smj1860/fieldstay
Length of output: 6540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/fixture.ts" <<'EOF'
async function verifyCoverage() {
const { data } = await supabase.from('items').select()
const { data: aliasedData } = await supabase.from('items').select()
let { data: letData } = await supabase.from('items').select()
var { count: varCount } = await supabase.from('items').select()
const [{ data: batchData }] = await Promise.all([
supabase.from('items').select(),
])
let [{ data: letBatchData }] = await Promise.all([
supabase.from('items').select(),
])
var [{ count: batchCount }] = await Promise.all([
supabase.from('items').select(),
])
const { data: handledData, error: handledError } =
await supabase.from('items').select()
const [{ count: handledCount, error: handledBatchError }] =
await Promise.all([supabase.from('items').select()])
}
EOF
cat > "$tmpdir/direct.yml" <<'EOF'
rules:
- id: single
languages: [typescript]
message: single
severity: ERROR
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"
- pattern-not: "const {..., error, ...} = await $Q"
- pattern-not: "const {..., error: $E, ...} = await $Q"
- metavariable-pattern:
metavariable: $Q
pattern: <... $S.from($T) ...>
- id: fanin
languages: [typescript]
message: fanin
severity: ERROR
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(...)"
EOF
semgrep --config "$tmpdir/direct.yml" --json "$tmpdir/fixture.ts" |
jq '[.results[] | {check_id, start_line: .start.line, matched: .extra.lines}]'Repository: smj1860/fieldstay
Length of output: 922
Expand both Supabase result-destructure rules.
The current rules report const data bindings but miss let and var, count-only bindings, and count aliases. Add matching patterns and error exclusions for these forms in both rules, including Promise.all fan-ins.
📍 Affects 1 file
.semgrep/chokepoints.yml#L398-L410(this comment).semgrep/chokepoints.yml#L428-L432
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.semgrep/chokepoints.yml around lines 398 - 410, Expand both Supabase
result-destructure rule groups at .semgrep/chokepoints.yml lines 398-410 and
428-432 to match let and var declarations, count-only destructuring, and aliased
count bindings in addition to current const data forms. Add corresponding
error-exclusion patterns for every new declaration shape, and ensure the
expanded patterns also cover Promise.all fan-ins; preserve the existing
exclusions for destructures containing error.
| 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 $$; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
: "${DATABASE_URL:?Set DATABASE_URL to a staging database URL}"
psql "$DATABASE_URL" -v ON_ERROR_STOP=1 -c "
SELECT conname, pg_get_constraintdef(oid)
FROM pg_constraint
WHERE conrelid = 'public.inventory_consumption_stats'::regclass
AND contype = 'p';
"Repository: smj1860/fieldstay
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="supabase/migrations/20260811020000_fix_par_stats_junction_ambiguity.sql"
printf '%s\n' '--- migration ---'
cat -n "$file"
printf '%s\n' '--- references to inventory_consumption_stats ---'
rg -n --hidden -g '!node_modules' -g '!dist' -g '!build' \
'inventory_consumption_stats|property_id' .
printf '%s\n' '--- related migration files ---'
git ls-files 'supabase/migrations/**/*.sql' | tail -n 30Repository: smj1860/fieldstay
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- handwritten inventory stats type ---'
sed -n '645,690p' types/database.ts
printf '%s\n' '--- generated inventory stats type ---'
sed -n '1825,1905p' types/database.generated.ts
printf '%s\n' '--- schema reference inventory stats ---'
rg -n -A45 -B8 'inventory_consumption_stats' supabase/schema_reference.sql
printf '%s\n' '--- exact table/column consumers ---'
rg -n --hidden -g '!node_modules' -g '!dist' -g '!build' \
'inventory_consumption_stats(\.|\b)|inventory_consumption_stats_' \
app components lib scripts unit e2e supabase types 2>/dev/null || true
printf '%s\n' '--- migrations defining or altering the table ---'
rg -n -A45 -B10 \
'CREATE TABLE[^;]*inventory_consumption_stats|ALTER TABLE[^;]*inventory_consumption_stats' \
supabase/migrations supabase/schema_reference.sqlRepository: smj1860/fieldstay
Length of output: 4423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migrations mentioning the table ---'
rg -l --hidden -g '*.sql' 'inventory_consumption_stats' supabase/migrations supabase/schema_reference.sql \
| sort
printf '%s\n' '--- table definition and constraints in matching migrations ---'
for file in $(rg -l --hidden -g '*.sql' 'inventory_consumption_stats' supabase/migrations supabase/schema_reference.sql | sort); do
printf '\n--- %s ---\n' "$file"
rg -n -A55 -B12 'inventory_consumption_stats' "$file"
done
printf '%s\n' '--- invariant checks for the table ---'
sed -n '175,230p' scripts/check-db-invariants.mjs
printf '%s\n' '--- relevant migration/type file status ---'
git status --short -- supabase/migrations/20260811020000_fix_par_stats_junction_ambiguity.sql types/database.ts types/database.generated.tsRepository: smj1860/fieldstay
Length of output: 11908
Do not suppress primary-key creation errors.
If a differently named primary key already exists, ADD CONSTRAINT can raise invalid_table_definition. The handler then reports success while retaining the unexpected key shape. Let the migration fail or validate the existing constraint explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260811020000_fix_par_stats_junction_ambiguity.sql`
around lines 46 - 50, Update the primary-key creation block around
inventory_consumption_stats_pkey to stop swallowing invalid_table_definition
errors. Let ADD CONSTRAINT fail when the existing primary key has an unexpected
shape, or explicitly inspect and validate the existing constraint before
proceeding; do not treat the failed creation as successful.
| /** | ||
| * 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the InventoryConsumptionStats table-grain documentation.
Lines 655-662 still state that the table has one row per (property, item) and a composite primary key. Lines 663-668 correctly state that inventory_item_id is the only key. Remove the obsolete description so later callers do not assume that property_id exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@types/database.ts` around lines 655 - 668, Update the
InventoryConsumptionStats table documentation by removing the obsolete
description that claims rows are keyed by (property, item) and use a composite
primary key. Retain the explanation that inventory_item_id is the sole primary
key and property_id is intentionally absent.



Summary by CodeRabbit
New Features
Documentation
Tests