feat: credit_grants - who set an account's balance by hand, and why (chat#1948) - #55
Conversation
…chat#1948) Topping up an account today means a direct write to credits_usage, which records nothing about who did it or what for. A grant is indistinguishable from a Stripe top-up or a monthly reset minutes afterwards. credits_usage is current state and a grant is an event, so this is a new table rather than columns there — columns would only ever hold the most recent grant. usage_events is the wrong home too: it is deduction-shaped (tokens, provider, model_id) with no actor and no reason column. The two constraints that carry the intent: granted_by is ON DELETE RESTRICT so attribution cannot be silently orphaned, and reason is CHECK-constrained non-empty after trimming so a blank reason is rejected by the schema rather than only by Zod. Implements item 2 of recoupable/chat#1948. Contract: recoupable/docs#295.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Updates to Preview Branch (feat/credit-grants) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
📝 WalkthroughWalkthroughThe migration adds ChangesCredit grants
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 `@supabase/migrations/20260806230000_create_credit_grants.sql`:
- Line 25: Update the credit grant creation contract associated with
credit_grants.id so grant requests require a caller-provided stable idempotency
key rather than relying on gen_random_uuid(). Mirror the existing p_event_id
contract used for credit deductions, and enforce the key through the API and
persistence path so retries reuse the same grant instead of creating duplicates.
- Around line 32-34: Update the reason CHECK constraint in the credit-grants
table definition to reject values containing only whitespace, including tabs and
newlines, rather than relying on btrim(reason). Preserve the NOT NULL
requirement and allow any reason containing at least one non-whitespace
character.
🪄 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: a4145e04-83d2-4359-befa-f25190f63246
📒 Files selected for processing (1)
supabase/migrations/20260806230000_create_credit_grants.sql
| -- the usage_events debits for the same account and period. | ||
|
|
||
| CREATE TABLE IF NOT EXISTS public.credit_grants ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 'credit_grants|admins/credits|idempotency|p_event_id' \
--glob '*.sql' --glob '*.ts' --glob '*.tsx' --glob '*.js'Repository: recoupable/database
Length of output: 157
🏁 Script executed:
#!/usr/bin/env bash
set -u
echo "Tracked files count:"
git ls-files | wc -l
echo
echo "Search all tracked files for relevant terms:"
git ls-files -z | xargs -0 rg -n -C 8 'credit_grants|admins/credits|idempotency|p_event_id|deduct_credits_with_audit' || true
echo
echo "Migrations containing credits/grants/audit:"
git ls-files 'supabase/migrations/*.sql' | while read -r f; do
if rg -q -i 'credit|grant|deduct|event_id' "$f"; then
printf '%s\n' "$f"
fi
doneRepository: recoupable/database
Length of output: 13892
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Supabase migration outline:"
wc -l supabase/migrations/20260806230000_create_credit_grants.sql
sed -n '1,140p' supabase/migrations/20260806230000_create_credit_grants.sql
echo
echo "Search for grant-related database objects without path restriction:"
git ls-files -z | xargs -0 rg -n -C 5 'credit_grants|grant.*credits|CreditsGrant|credits_grants|p_event_id|event_id' || trueRepository: recoupable/database
Length of output: 11865
Require a stable grant request id within the API.
credit_grants.id auto-generates via gen_random_uuid() when omitted. A retried POST /api/admins/credits without a stable request key can create a second grant and apply the same credits twice. Define and enforce an API-level idempotency key for grants, similar to the p_event_id contract for credit deductions.
🤖 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/20260806230000_create_credit_grants.sql` at line 25,
Update the credit grant creation contract associated with credit_grants.id so
grant requests require a caller-provided stable idempotency key rather than
relying on gen_random_uuid(). Mirror the existing p_event_id contract used for
credit deductions, and enforce the key through the API and persistence path so
retries reuse the same grant instead of creating duplicates.
| -- Non-empty after trimming: a blank reason is the failure mode this table | ||
| -- exists to prevent, so it is rejected in the schema and not only in Zod. | ||
| reason TEXT NOT NULL CHECK (btrim(reason) <> ''), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
psql "$DATABASE_URL" -v ON_ERROR_STOP=1 <<'SQL'
WITH inputs(reason) AS (
VALUES (E' '), (E'\t'), (E'\n')
)
SELECT reason,
btrim(reason) <> '' AS current_check,
reason ~ '[^[:space:]]' AS whitespace_aware_check
FROM inputs;
SQLRepository: recoupable/database
Length of output: 202
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Migration excerpt:"
sed -n '1,80p' supabase/migrations/20260806230000_create_credit_grants.sql
echo
echo "Search for reason validation references:"
rg -n "reason|credit_grants|btrim\\(reason\\)|\\[^:\\[: space\\]\\]|no_reason" -S . || true
echo
echo "PostgreSQL btrim/regex behavior from installed client docs if unavailable:"
if command -v psql >/dev/null 2>&1; then
psql --version
else
echo "psql not installed"
fiRepository: recoupable/database
Length of output: 4574
🌐 Web query:
PostgreSQL single-argument btrim whitespace Unicode spaces implementation
💡 Result:
In PostgreSQL, the single-argument version of the btrim function is designed to trim only the standard ASCII space character (Unicode U+0020, chr(32)) from the beginning and end of a string [1][2][3]. It does not automatically recognize or remove other Unicode whitespace characters, such as non-breaking spaces (U+00A0), tabs, or various other Unicode space separators [3][4]. Implementation Details: The btrim function (along with ltrim and rtrim) is implemented in the PostgreSQL source code (specifically in src/backend/utils/adt/oracle_compat.c) [5][6]. When called with a single argument, the function internally invokes the trimming logic using a hardcoded space character (' ') as the set of characters to be removed [7]. Because it relies on this specific single-character set, it is limited to removing only that exact byte or character value [5][6]. To trim other types of Unicode whitespace, you must explicitly specify them in the optional second argument of btrim or use alternative approaches such as regular expressions [3][4]. For example, to remove a wider range of whitespace characters, you can use: -- Example using btrim with a custom set SELECT btrim(your_column, ' ' || chr(160) || chr(9)); -- Example using regexp_replace to remove various Unicode whitespace SELECT regexp_replace(your_column, '^[\s\u00A0]+|[\s\u00A0]+$', '', 'g'); Note that PostgreSQL's POSIX regular expressions use the [:space:] character class, which covers standard ASCII whitespace (space, tab, line feed, carriage return, etc.) [3][4], but even this may require manual additions (like \u00A0) to cover all Unicode space separators, as support for advanced Unicode properties (like \p{Zs}) is not natively available in PostgreSQL regular expression syntax [3][4].
Citations:
- 1: https://www.postgresql.org/docs/current/functions-string.html
- 2: https://www.postgresql.org/docs/16/functions-string.html
- 3: https://dba.stackexchange.com/questions/345484/nbsp-non-breaking-space-is-not-considered-whitespace-by-postgres
- 4: https://stackoverflow.com/questions/63302656/remove-all-unicode-space-separators-in-postgresql
- 5: https://doxygen.postgresql.org/oracle__compat_8c_source.html
- 6: https://github.com/postgres/postgres/blob/92268b35d04c2de416279f187d12f264afa22614/src/backend/utils/adt/oracle_compat.c
- 7: https://docs.huihoo.com/doxygen/postgresql/oracle__compat_8c.html
🌐 Web query:
PostgreSQL regular expression [:space:] trim whitespace single quote literal documentation
💡 Result:
In PostgreSQL, you can use the POSIX character class [[:space:]] within regular expression functions like regexp_replace to match and trim whitespace [1][2]. Trimming Whitespace with Regular Expressions To remove leading and trailing whitespace using [[:space:]], you can use the regexp_replace function [1]: SELECT regexp_replace(' text with spaces ', '^[[:space:]]+|[[:space:]]+$', '', 'g'); The 'g' flag ensures that all matches are replaced globally [1][3]. Note that [[:space:]] covers standard whitespace characters such as spaces, tabs, and newlines [2]. If you need to handle specific Unicode whitespace characters beyond standard ASCII, you may need to define a custom character class, as [[:space:]] does not encompass all Unicode space separators [1][4]. Handling Single Quotes in Literal Strings PostgreSQL string constants are defined using single quotes [5]. To include a literal single quote within a string, you must escape it by doubling it ('') [6][7]. For example, to use a regex pattern that includes a single quote, you would write: SELECT regexp_replace(my_column, '^'[[:space:]]+', '', 'g'); String Literals and Backslashes PostgreSQL handles backslashes in string literals differently based on the standard_conforming_strings configuration [8]. By default (on), backslashes are treated as literal characters [8]. If you need to use backslash-based escape sequences (like \s for whitespace) within a string literal, you should use the escape string syntax by prefixing the string with E (e.g., E'\s+') [6][9][8]. When using escape strings, remember that a single backslash in the regex (\s) must be represented as a double backslash (\s) in the literal string [3]. Summary Table Action | Syntax --- | --- Standard trim | trim(both from string) [6] Regex trim | regexp_replace(string, '^[[:space:]]+|[[:space:]]+$', '', 'g') [1] Escape single quote | '' (double single quote) [7] Regex escape string | E'[[:space:]]+' (prefix with E) [9][8]
Citations:
- 1: https://stackoverflow.com/questions/22699535/trim-trailing-spaces-with-postgresql
- 2: https://www.postgresql.org/docs/current/functions-matching.html
- 3: https://www.regular-expressions.info/postgresql.html
- 4: https://stackoverflow.com/questions/63302656/remove-all-unicode-space-separators-in-postgresql
- 5: https://stackoverflow.com/questions/41396195/what-is-the-difference-between-single-quotes-and-double-quotes-in-postgresql
- 6: https://www.postgresql.org/docs/current/functions-string.html
- 7: https://stackoverflow.com/questions/12316953/insert-text-with-single-quotes-in-postgresql
- 8: https://postgresqlco.nf/doc/en/param/standard_conforming_strings/
- 9: https://stackoverflow.com/questions/935/string-literals-and-escape-characters-in-postgresql
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Supabase migration list:"
fd -a '^202[0-9]{8}[0-9]{6}_' supabase/migrations 2>/dev/null | sort -n | sed -n '1,120p' || true
echo
echo "Supabase Dockerfile/package references:"
fd -a 'Dockerfile|docker-compose|supabase|package.json' . 2>/dev/null | sed -n '1,120p' || true
echo
echo "Search for Supabase migration config:"
rg -n "version|extensions|pg15|pg17|postgres|supabase|migrations" -S . --glob '!node_modules' --glob '!dist' --glob '!build' 2>/dev/null | sed -n '1,200p' || trueRepository: recoupable/database
Length of output: 3396
🌐 Web query:
PostgreSQL btrim builtin behavior space character source Oracle_compat c
💡 Result:
In PostgreSQL, the builtin btrim(string, characters) function removes the longest string containing only the characters specified in the characters argument from both the start and end of the input string [1][2]. If the characters argument is omitted, the function defaults to removing space characters [1][3]. It is important to note that the characters argument is treated as a set of individual characters to be removed, not as a literal substring [4][2]. The function will continue to strip any combination of these characters from both ends until it encounters a character that is not in the specified set [4][2]. While PostgreSQL includes a file named oracle_compat.c in its source code, which contains implementations intended to improve compatibility with Oracle syntax, the standard builtin btrim function is native to PostgreSQL and follows the behavior described above [5]. For users specifically requiring Oracle-style compatibility for string trimming functions—as the behavior of trimming can sometimes differ between PostgreSQL and Oracle, especially regarding types like CHAR—the community project orafce provides an oracle.btrim function [6][7]. To utilize this, the orafce extension must be installed, and the search_path should be configured to place the oracle schema before pg_catalog [6][7].
Citations:
- 1: https://www.postgresql.org/docs/16/functions-string.html
- 2: https://database.guide/understanding-the-btrim-function-in-postgresql/
- 3: https://www.pgtutorial.com/postgresql-string-functions/postgresql-btrim/
- 4: https://stackoverflow.com/questions/48136303/i-cant-understand-the-behaviour-of-btrim
- 5: https://doxygen.postgresql.org/oracle__compat_8c.html
- 6: https://github.com/orafce/orafce/
- 7: https://pgxn.org/dist/orafce/doc/orafce_documentation/Orafce_Documentation_02.html
Reject all-whitespace reason values.
btrim(reason) only drops space (U+0020), so tab-only, newline-only, or other whitespace-only reasons can still satisfy btrim(reason) <> ''. Use a whitespace-aware constraint if blank means any whitespace.
Proposed constraint
- reason TEXT NOT NULL CHECK (btrim(reason) <> ''),
+ reason TEXT NOT NULL CHECK (reason ~ '[^[:space:]]'),📝 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.
| -- Non-empty after trimming: a blank reason is the failure mode this table | |
| -- exists to prevent, so it is rejected in the schema and not only in Zod. | |
| reason TEXT NOT NULL CHECK (btrim(reason) <> ''), | |
| -- Non-empty after trimming: a blank reason is the failure mode this table | |
| -- exists to prevent, so it is rejected in the schema and not only in Zod. | |
| reason TEXT NOT NULL CHECK (reason ~ '[^[:space:]]'), |
🤖 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/20260806230000_create_credit_grants.sql` around lines 32
- 34, Update the reason CHECK constraint in the credit-grants table definition
to reject values containing only whitespace, including tabs and newlines, rather
than relying on btrim(reason). Preserve the NOT NULL requirement and allow any
reason containing at least one non-whitespace character.
…hat#1948) Two separate PostgREST calls (move the balance, then insert the grant) can drift on partial failure. deduct_credits_with_audit exists for that exact reason on the debit side. Here the drift is worse than an accounting hiccup: a moved balance with no grant row is the untraceable write this change exists to eliminate, and would be indistinguishable from the hand-written database updates being replaced. A plpgsql body runs in an implicit transaction. previous_credits is captured inside the function so it cannot be stale by the time the balance moves, and the timestamp bump that restarts the monthly-reset clock happens in the same transaction as the grant.
|
Pushed Moving the balance and recording the grant are two writes. Through PostgREST they are not atomic and can drift on partial failure — which is why Two details worth a look:
Account existence stays an API concern (404 for an unknown |
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="supabase/migrations/20260806230000_create_credit_grants.sql">
<violation number="1" location="supabase/migrations/20260806230000_create_credit_grants.sql:34">
P2: Tab- or newline-only reasons are accepted, so the audit table can still contain a visually blank reason despite the documented non-empty constraint. Use a whitespace-aware predicate so every whitespace-only reason is rejected.</violation>
<violation number="2" location="supabase/migrations/20260806230000_create_credit_grants.sql:37">
P3: The audit column previous_credits records the balance before the grant, but unlike the sibling remaining_credits column it has no CHECK (previous_credits >= 0). A credit balance must never be negative, and this migration's stated philosophy is to enforce grant invariants in the schema rather than relying on the API layer (the same reason the reason column and remaining_credits got CHECKs). Consider adding CHECK (previous_credits >= 0) for consistency and to keep a malformed grant from polluting the audit trail.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| granted_by UUID NOT NULL REFERENCES public.accounts(id) ON DELETE RESTRICT, | ||
| -- Non-empty after trimming: a blank reason is the failure mode this table | ||
| -- exists to prevent, so it is rejected in the schema and not only in Zod. | ||
| reason TEXT NOT NULL CHECK (btrim(reason) <> ''), |
There was a problem hiding this comment.
P2: Tab- or newline-only reasons are accepted, so the audit table can still contain a visually blank reason despite the documented non-empty constraint. Use a whitespace-aware predicate so every whitespace-only reason is rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260806230000_create_credit_grants.sql, line 34:
<comment>Tab- or newline-only reasons are accepted, so the audit table can still contain a visually blank reason despite the documented non-empty constraint. Use a whitespace-aware predicate so every whitespace-only reason is rejected.</comment>
<file context>
@@ -0,0 +1,46 @@
+ granted_by UUID NOT NULL REFERENCES public.accounts(id) ON DELETE RESTRICT,
+ -- Non-empty after trimming: a blank reason is the failure mode this table
+ -- exists to prevent, so it is rejected in the schema and not only in Zod.
+ reason TEXT NOT NULL CHECK (btrim(reason) <> ''),
+ -- Balance immediately before the grant. NULL when the account had no
+ -- credits_usage row at all and the grant created one.
</file context>
| reason TEXT NOT NULL CHECK (btrim(reason) <> ''), | |
| reason TEXT NOT NULL CHECK (reason ~ '[^[:space:]]'), |
| reason TEXT NOT NULL CHECK (btrim(reason) <> ''), | ||
| -- Balance immediately before the grant. NULL when the account had no | ||
| -- credits_usage row at all and the grant created one. | ||
| previous_credits INTEGER, |
There was a problem hiding this comment.
P3: The audit column previous_credits records the balance before the grant, but unlike the sibling remaining_credits column it has no CHECK (previous_credits >= 0). A credit balance must never be negative, and this migration's stated philosophy is to enforce grant invariants in the schema rather than relying on the API layer (the same reason the reason column and remaining_credits got CHECKs). Consider adding CHECK (previous_credits >= 0) for consistency and to keep a malformed grant from polluting the audit trail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260806230000_create_credit_grants.sql, line 37:
<comment>The audit column previous_credits records the balance before the grant, but unlike the sibling remaining_credits column it has no CHECK (previous_credits >= 0). A credit balance must never be negative, and this migration's stated philosophy is to enforce grant invariants in the schema rather than relying on the API layer (the same reason the reason column and remaining_credits got CHECKs). Consider adding CHECK (previous_credits >= 0) for consistency and to keep a malformed grant from polluting the audit trail.</comment>
<file context>
@@ -0,0 +1,46 @@
+ reason TEXT NOT NULL CHECK (btrim(reason) <> ''),
+ -- Balance immediately before the grant. NULL when the account had no
+ -- credits_usage row at all and the grant created one.
+ previous_credits INTEGER,
+ -- Balance the account was left holding. Absolute, not a delta.
+ remaining_credits INTEGER NOT NULL CHECK (remaining_credits >= 0),
</file context>
There was a problem hiding this comment.
4 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="supabase/migrations/20260806230000_create_credit_grants.sql">
<violation number="1" location="supabase/migrations/20260806230000_create_credit_grants.sql:71">
P2: grant_credits_with_audit has no idempotency key parameter, so a retried call (e.g. from a retried API request) could insert a duplicate credit_grants row and re-apply the balance change. Consider adding a stable request/grant id parameter, similar to the p_event_id contract used by deduct_credits_with_audit, so retries are safely deduplicated.</violation>
<violation number="2" location="supabase/migrations/20260806230000_create_credit_grants.sql:78">
P0: `grant_credits_with_audit` is SECURITY DEFINER (runs as the function owner, bypassing RLS) but this migration never restricts who may execute it. New functions default to EXECUTE for PUBLIC, so the `anon` role exposed through PostgREST can call it to set any account's balance to any value and record a grant attributed to any `granted_by` — exactly the untraceable, unauthenticated write this feature is meant to eliminate. The accompanying `deduct_credits_with_audit` and `get_credit_spend_digest` functions in this repo restrict EXECUTE to `authenticated`/`service_role`; this one should do the same.</violation>
<violation number="3" location="supabase/migrations/20260806230000_create_credit_grants.sql:86">
P1: The grant RPC cannot run on a database initialized from this migration chain because `credits_usage.remaining_credits` is never created. Adding that column in a migration (with the intended nullability and constraints) before using it would make the function executable.</violation>
<violation number="4" location="supabase/migrations/20260806230000_create_credit_grants.sql:90">
P1: Concurrent grants can record an incorrect `previous_credits` and can create multiple balance rows for one account, undermining the audit trail and current-balance state. Enforcing one `credits_usage` row per account and serializing the read/update (for example by locking the account row) would make the operation consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| p_remaining_credits integer | ||
| ) RETURNS public.credit_grants | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER |
There was a problem hiding this comment.
P0: grant_credits_with_audit is SECURITY DEFINER (runs as the function owner, bypassing RLS) but this migration never restricts who may execute it. New functions default to EXECUTE for PUBLIC, so the anon role exposed through PostgREST can call it to set any account's balance to any value and record a grant attributed to any granted_by — exactly the untraceable, unauthenticated write this feature is meant to eliminate. The accompanying deduct_credits_with_audit and get_credit_spend_digest functions in this repo restrict EXECUTE to authenticated/service_role; this one should do the same.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260806230000_create_credit_grants.sql, line 78:
<comment>`grant_credits_with_audit` is SECURITY DEFINER (runs as the function owner, bypassing RLS) but this migration never restricts who may execute it. New functions default to EXECUTE for PUBLIC, so the `anon` role exposed through PostgREST can call it to set any account's balance to any value and record a grant attributed to any `granted_by` — exactly the untraceable, unauthenticated write this feature is meant to eliminate. The accompanying `deduct_credits_with_audit` and `get_credit_spend_digest` functions in this repo restrict EXECUTE to `authenticated`/`service_role`; this one should do the same.</comment>
<file context>
@@ -44,3 +44,82 @@ CREATE TABLE IF NOT EXISTS public.credit_grants (
+ p_remaining_credits integer
+) RETURNS public.credit_grants
+ LANGUAGE plpgsql
+ SECURITY DEFINER
+ SET search_path = public, pg_temp
+AS $$
</file context>
| INTO v_previous | ||
| FROM public.credits_usage | ||
| WHERE account_id = p_account_id | ||
| LIMIT 1; |
There was a problem hiding this comment.
P1: Concurrent grants can record an incorrect previous_credits and can create multiple balance rows for one account, undermining the audit trail and current-balance state. Enforcing one credits_usage row per account and serializing the read/update (for example by locking the account row) would make the operation consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260806230000_create_credit_grants.sql, line 90:
<comment>Concurrent grants can record an incorrect `previous_credits` and can create multiple balance rows for one account, undermining the audit trail and current-balance state. Enforcing one `credits_usage` row per account and serializing the read/update (for example by locking the account row) would make the operation consistent.</comment>
<file context>
@@ -44,3 +44,82 @@ CREATE TABLE IF NOT EXISTS public.credit_grants (
+ INTO v_previous
+ FROM public.credits_usage
+ WHERE account_id = p_account_id
+ LIMIT 1;
+
+ IF FOUND THEN
</file context>
| v_now timestamptz := now(); | ||
| v_grant public.credit_grants; | ||
| BEGIN | ||
| SELECT remaining_credits |
There was a problem hiding this comment.
P1: The grant RPC cannot run on a database initialized from this migration chain because credits_usage.remaining_credits is never created. Adding that column in a migration (with the intended nullability and constraints) before using it would make the function executable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260806230000_create_credit_grants.sql, line 86:
<comment>The grant RPC cannot run on a database initialized from this migration chain because `credits_usage.remaining_credits` is never created. Adding that column in a migration (with the intended nullability and constraints) before using it would make the function executable.</comment>
<file context>
@@ -44,3 +44,82 @@ CREATE TABLE IF NOT EXISTS public.credit_grants (
+ v_now timestamptz := now();
+ v_grant public.credit_grants;
+BEGIN
+ SELECT remaining_credits
+ INTO v_previous
+ FROM public.credits_usage
</file context>
| -- p_reason why (non-empty after trimming, per the CHECK above) | ||
| -- p_remaining_credits balance to leave the account holding — absolute, not a delta | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.grant_credits_with_audit( |
There was a problem hiding this comment.
P2: grant_credits_with_audit has no idempotency key parameter, so a retried call (e.g. from a retried API request) could insert a duplicate credit_grants row and re-apply the balance change. Consider adding a stable request/grant id parameter, similar to the p_event_id contract used by deduct_credits_with_audit, so retries are safely deduplicated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260806230000_create_credit_grants.sql, line 71:
<comment>grant_credits_with_audit has no idempotency key parameter, so a retried call (e.g. from a retried API request) could insert a duplicate credit_grants row and re-apply the balance change. Consider adding a stable request/grant id parameter, similar to the p_event_id contract used by deduct_credits_with_audit, so retries are safely deduplicated.</comment>
<file context>
@@ -44,3 +44,82 @@ CREATE TABLE IF NOT EXISTS public.credit_grants (
+-- p_reason why (non-empty after trimming, per the CHECK above)
+-- p_remaining_credits balance to leave the account holding — absolute, not a delta
+
+CREATE OR REPLACE FUNCTION public.grant_credits_with_audit(
+ p_account_id uuid,
+ p_granted_by uuid,
</file context>
… (chat#1948) (#824) * feat(admins): POST /api/admins/credits sets a balance and records why (chat#1948) Every route under /api/admins/credits was a GET, so topping up an account meant a direct database write with no record of who did it or why. 155 accounts sit at or below zero with no supported way to unblock any of them. - POST /api/admins/credits sets an absolute balance, gated on Recoup org membership via validateAdminAuth. reason is required and non-empty; granted_by comes from credentials and a granted_by in the body is ignored. - The balance move and the grant record go through the new grant_credits_with_audit function so they cannot drift apart. A balance that moved without the grant explaining it is the exact failure this endpoint exists to remove. - The response carries expires_at, so the one-month reset window is a value the caller can act on rather than a footnote. - GET /api/admins/credits/events returns a grants array beside events, so a hand-set balance is attributable through the endpoint that already answers "where did this account's credits go". - insertCreditsUsage takes an optional timestamp so a grant that creates the first credits row starts the refill clock at a known moment. Omitted by every existing caller, whose behaviour is unchanged. Implements items 1-3 of recoupable/chat#1948. Contract: recoupable/docs#295. Schema: recoupable/database#55. * fix(credits): clamp grant expiry, drop dead param, stabilise grant order Preview verification and bot review turned up three real defects. 1. expires_at overstated the guarantee for month-end grants. setMonth(+1) is not the inverse of the reset's setMonth(-1): a Jan 31 grant reported Mar 3 while the reset actually fires around Mar 2, so an admin was told the balance was safe a day longer than it was. Now clamped to the last day of the target month (Jan 31 -> Feb 28), which is at or before the real reset in every case. Erring late is the only direction that misleads, so the tests pin that as a property across six dates, checked against a replica of the refill predicate. UTC accessors so the result does not move with the host timezone. (cubic P2) 2. insertCreditsUsage's optional timestamp was dead code. It was written before the INSERT branch moved into grant_credits_with_audit; nothing calls it with a timestamp now. Reverted the file and dropped its test. This also moots the truthiness-vs-undefined comment on the same line. (cubic P3, CodeRabbit) 3. selectCreditGrants ordered only by created_at, so grants written in the same instant came back in a non-deterministic order, which near the 500-row cap changes which rows return at all. Added the id DESC tiebreaker the sibling selectUsageEvents already has. (cubic P3) Also pins the 400 envelope in the validator tests, including missing_fields being absent on unparseable JSON, so a field moving to message cannot pass a status-only assertion. (cubic P3) Verified live on the preview before these changes; re-verifying after.
The table behind the staff-facing credit grant. Second of three PRs for chat#1948.
Merge order: recoupable/docs#295 → this PR → recoupable/api (the endpoint). This one is a hard dependency of the api PR, which reads and writes the table.
Why
Every route under
/api/admins/creditsis aGET, and the only writers tocredits_usageare internal libs reachable from the Stripe webhook and the credits read path. So a top-up today is a direct database write that leaves no record of who made it or why — indistinguishable from a Stripe top-up or a monthly reset, minutes later. 155 accounts are at or below zero with no supported way for staff to unblock any of them.Auditability is the requirement here; the endpoint is just the mechanism. An endpoint that wrote the same untraceable row would only have made the untraceable write easier.
Why a new table
credits_usageusage_eventsinput_tokens,provider,model_id,credits_deducted_cents. No actor column, no reason column. A grant has no model and a debit has no human.Same state-vs-event split argued in chat#1947.
Constraints worth reviewing
granted_byisON DELETE RESTRICT.SET NULLwould quietly destroy the attribution the table exists to hold. Deleting a Recoup staff account that has granted credits should fail loudly and be dealt with. The cost is real but small:deleteAccountByIdwill now reject such an account, and in practice only staff accounts ever appear in this column.account_idisON DELETE CASCADE, matchingcredits_usageandusage_events— the balance the grant describes would itself be gone.reasonisCHECK (btrim(reason) <> ''). Zod will reject a blank reason too, but the empty-reason grant is the exact failure mode this table exists to prevent, so it is enforced in the schema as well as at the edge.No RLS, consistent with
catalog_valuationsand the other api-written tables — reads and writes go through the service-role client.Implements item 2 of recoupable/chat#1948.
Summary by cubic
Added
public.credit_grantsandpublic.grant_credits_with_audit()to audit staff credit grants and make balance updates atomic, capturing who did it, why, and the before/after balances. Implements item 2 of recoupable/chat#1948 and is a hard dependency for the upcoming admin credits API.New Features
id,account_id,granted_by,reason,previous_credits,remaining_credits,created_at.granted_byON DELETE RESTRICT;account_idON DELETE CASCADE;reasonmust be non-empty;remaining_credits>= 0.(account_id, created_at DESC)for recent-per-account queries.public.grant_credits_with_audit(account_id, granted_by, reason, remaining_credits)setscredits_usageand inserts a grant in one transaction, capturesprevious_credits, and bumpscredits_usage.timestamp.Dependencies
recoupable/docs#295→ this PR →recoupable/api.POST /api/admins/creditsand read viaGET /api/admins/credits/events.Written for commit 09b040a. Summary will update on new commits.
Summary by CodeRabbit