feat(integrity): schema, migration, and shared helpers for audit fixes#34
feat(integrity): schema, migration, and shared helpers for audit fixes#34projectamazonph wants to merge 4 commits into
Conversation
Foundation for the 2026-07-17 audit remediation (integrity + security findings that apply to main): - Add XpLedger with unique (userId, eventKey) for exactly-once XP (C10). - Add User.claimTokenHash + claimTokenExpiresAt for guest-account claiming (C5/C6). - Make RefundRequest.paymongoRefundId unique (C8). - Raw-SQL partial unique indexes: one active refund per payment (C7), one active certificate per (user, course) (H7). - Backfill existing emails to canonical lowercase, collision-safe (H6). - Shared helpers: awardXpOnce (idempotent XP), isUniqueConstraintError, claim-token mint/hash. See docs/AUDIT-REMEDIATION-2026-07-17.md for scope and deferred items. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoQ1YGTKVSky148W8EAr9Z
- markLessonCompleteAction / submitQuizAction award XP via awardXpOnce so concurrent completions and re-submits grant XP exactly once; quiz attemptNumber retries on unique conflict instead of count+1 (C10). - Block manual completion of quiz-gated lessons server-side and hide the manual button in the lesson page (H1). - submitToolSession grades first (bad state stays retriable), then atomically claims the terminal state and awards XP in one transaction; saveToolSession rejects post-submission edits (H3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoQ1YGTKVSky148W8EAr9Z
- Refund single-writer: Payment.refundAmountPhp derived from the sum of PROCESSED requests by both admin and webhook paths, so a refund is counted once; only full refunds revoke enrollment (C8). - Approve path splits provider call from local reconciliation: a DB failure after a successful PayMongo refund reports PROCESSING, never a provider failure, and never re-refunds (C9). - Refund-request creation maps the one-active-per-payment unique index to a friendly error (C7). - Discount redemption uses a guarded conditional increment (H4). - Guest-checkout account claiming: mint a single-use hashed claim token, email a dedicated claim link after commit, require + atomically consume it at signup (C5/C6). Checkout-complete guests are pointed to the email instead of a tokenless signup. - Canonicalize emails (trim + lowercase) at signup/signin/checkout (H6). - Certificates count only published lessons and are race-safe against the one-active-per-course index (H7). - Sentry tracing uses startSpan (v8) with a per-invocation timer (H9). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoQ1YGTKVSky148W8EAr9Z
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR applies audit remediation across guest account claiming, XP awarding, refunds, enrollment, certificates, email normalization, lesson completion, tool sessions, tracing, and database integrity constraints. It adds targeted tests and documents deferred audit items. ChangesAudit integrity remediation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Guest
participant Checkout
participant Email
participant Signup
participant Database
Guest->>Checkout: complete guest payment
Checkout->>Database: create placeholder claim record
Checkout->>Email: send single-use claim link
Email-->>Guest: claim link
Guest->>Signup: submit claim form
Signup->>Database: atomically validate and consume claim
Database-->>Signup: return claimed account
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/actions/tools.ts (1)
95-108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the freeze check atomic with the save.
Submission can transition the session after Line 98 but before the unconditional update at Line 102, allowing a late save to overwrite the state that was graded. Use
updateManyguarded bystatus: 'IN_PROGRESS'and reject when its count is zero.🤖 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 `@src/app/actions/tools.ts` around lines 95 - 108, Make the session freeze check atomic in the save flow around the existing status validation and db.toolSession.update call. Replace the separate status check and unconditional update with updateMany guarded by both the session id and status: 'IN_PROGRESS', then reject with the existing submitted-session error when the result count is zero; preserve the current state and timeSpentSeconds values.src/app/actions/refunds.ts (1)
237-254: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftDo not mark ambiguous PayMongo refund errors as FAILED.
src/app/actions/refunds.ts:237-254refundPayment()drops PayMongo status and error-code details, so a timeout or connection reset is indistinguishable from a real rejection here. BecauseFAILEDis not blocking, that can reopen the request and allow another refund to be issued. Keep outcome-unknown cases inAPPROVEDand retry with anIdempotency-Keybased onrequest.id; only explicit 4xx rejections should transition toFAILED.🤖 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 `@src/app/actions/refunds.ts` around lines 237 - 254, The refundPayment error path currently marks every PayMongo exception as FAILED, including ambiguous transport errors. Preserve APPROVED for timeouts, connection resets, and other unknown outcomes, retrying with an Idempotency-Key derived from request.id; transition to FAILED only when PayMongo status and error details identify an explicit 4xx rejection, while retaining the existing failure metadata and revalidation for confirmed rejections.src/app/(public)/auth/signup/SignUpForm.tsx (1)
57-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLock the email field while processing a claim token.
The claim is bound to the emailed placeholder account. If the buyer edits this field, the action can create a separate account while leaving the paid enrollment unclaimed. Render the email as read-only when
claimTokenis present.Proposed fix
<Input label="Email" type="email" name="email" autoComplete="email" required defaultValue={prefilledEmail} - hint={prefilledEmail ? 'From your checkout. Change if needed.' : undefined} + readOnly={Boolean(claimToken)} + hint={ + claimToken + ? 'This claim link is tied to this email.' + : prefilledEmail + ? 'From your checkout. Change if needed.' + : undefined + }🤖 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 `@src/app/`(public)/auth/signup/SignUpForm.tsx around lines 57 - 66, Update the email Input in SignUpForm so it is read-only whenever claimToken is present, preventing edits to the prefilled claim email while preserving normal editing when no claim token exists.
🧹 Nitpick comments (5)
docs/AUDIT-REMEDIATION-2026-07-17.md (1)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a plain-language introduction and define “CI.”
Replace “Second-pass deep audit response” with a concrete summary, and expand “CI” as “continuous integration” on first use.
As per coding guidelines, “Use direct, plain-spoken language for the Filipino VA audience, define jargon, and avoid generic AI-slop phrases.”
🤖 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 `@docs/AUDIT-REMEDIATION-2026-07-17.md` around lines 3 - 6, Rewrite the opening of the audit document with a concrete, plain-language summary of what this branch fixes instead of “Second-pass deep audit response,” and expand “CI” to “continuous integration” at its first use. Keep the scope and verification limitations clear while avoiding generic or jargon-heavy wording.Source: Coding guidelines
src/lib/xp.ts (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace em dashes in the changed comments.
src/lib/xp.ts#L8-L13: replace the em dash.src/lib/xp.ts#L39-L39: replace the em dash.src/app/actions/progress.ts#L97-L100: replace the em dash.src/app/actions/tools.ts#L95-L99: replace the em dash.src/app/actions/tools.ts#L139-L141: replace the em dash.src/app/actions/tools.ts#L176-L178: replace the em dash.src/app/actions/tools.ts#L232-L235: replace the em dash.src/lib/__tests__/xp.test.ts#L47-L49: replace the em dash.src/app/(dashboard)/courses/[courseSlug]/lessons/[lessonSlug]/page.tsx#L85-L87: replace the em dash.src/app/actions/refunds.ts#L224-L227: replace the em dash.src/app/actions/refunds.ts#L257-L265: replace the em dashes.src/app/actions/refunds.ts#L299-L299: replace the em dash.As per coding guidelines, “Do not use em-dashes. Use periods, commas, or parentheses instead.”
🤖 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 `@src/lib/xp.ts` around lines 8 - 13, Replace every em dash in the specified comments with periods, commas, or parentheses, preserving the existing meaning. Update src/lib/xp.ts lines 8-13 and 39, src/app/actions/progress.ts lines 97-100, src/app/actions/tools.ts lines 95-99, 139-141, 176-178, and 232-235, src/lib/__tests__/xp.test.ts lines 47-49, src/app/(dashboard)/courses/[courseSlug]/lessons/[lessonSlug]/page.tsx lines 85-87, and src/app/actions/refunds.ts lines 224-227, 257-265, and 299.Source: Coding guidelines
src/lib/__tests__/xp.test.ts (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep these tests next to the modules they test.
src/lib/__tests__/xp.test.ts#L1-L12: move the suite tosrc/lib/xp.test.ts.src/app/actions/__tests__/auth-actions.test.ts#L96-L130: move or merge these cases intosrc/app/actions/auth.test.ts.As per coding guidelines, “Keep tests next to the code they test: foo.ts should have foo.test.ts.”
🤖 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 `@src/lib/__tests__/xp.test.ts` around lines 1 - 12, Move the XP test suite from src/lib/__tests__/xp.test.ts to src/lib/xp.test.ts, keeping its awardXpOnce coverage and setup intact. Move or merge the affected authentication cases from src/app/actions/__tests__/auth-actions.test.ts lines 96-130 into src/app/actions/auth.test.ts, preserving their existing behavior and assertions.Source: Coding guidelines
src/lib/enrollment.ts (1)
151-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
PLACEHOLDER_PASSWORD_PREFIXwhen creating placeholder hashes.The claim action relies on that exported marker. Hardcoding the value here allows the creation and detection contracts to diverge.
Proposed fix
-import { generateClaimToken } from './claim-token'; +import { + generateClaimToken, + PLACEHOLDER_PASSWORD_PREFIX, +} from './claim-token'; - passwordHash: `placeholder_${randomUUID()}`, + passwordHash: `${PLACEHOLDER_PASSWORD_PREFIX}${randomUUID()}`,🤖 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 `@src/lib/enrollment.ts` around lines 151 - 162, Update the placeholder passwordHash assignment in the enrollment user creation flow to build the value with the exported PLACEHOLDER_PASSWORD_PREFIX constant instead of hardcoding "placeholder_"; preserve the existing randomUUID suffix and claim-token behavior.src/lib/claim-token.ts (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace em dashes in the changed code and copy.
src/lib/claim-token.ts#L21-L21: replace the em dash with a period or comma.src/lib/email.tsx#L175-L178: replace the em dash in the comment.src/lib/email.tsx#L256-L258: replace the em dash in customer-facing copy.src/lib/enrollment.ts#L359-L361: replace the em dash in the comment.src/lib/enrollment.ts#L455-L460: replace the em dash in the comment.src/app/(public)/checkout/complete/page.tsx#L66-L69: replace the em dash in the comment.src/app/(public)/checkout/complete/page.tsx#L119-L123: replace the em dash in customer-facing copy.src/app/actions/auth.ts#L44-L49: replace the em dash in the comment.src/lib/refunds.ts#L75-L81: replace the em dash in the comment.As per coding guidelines, code must not use em dashes.
🤖 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 `@src/lib/claim-token.ts` at line 21, Replace each em dash with a period or comma at the specified sites: src/lib/claim-token.ts lines 21-21; src/lib/email.tsx lines 175-178 and 256-258; src/lib/enrollment.ts lines 359-361 and 455-460; src/app/(public)/checkout/complete/page.tsx lines 66-69 and 119-123; src/app/actions/auth.ts lines 44-49; and src/lib/refunds.ts lines 75-81. Preserve the existing meaning of comments and customer-facing copy.Source: Coding guidelines
🤖 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 `@prisma/migrations/20260717000000_audit_integrity_fixes/migration.sql`:
- Around line 61-68: Update the email normalization UPDATE so it only
canonicalizes a row when no other user already has, or will normalize to, the
same email value. Adjust the collision check around the User migration query to
compare each other row’s canonicalized email (not only o.email) against
lower(btrim(u.email)), preserving one existing canonical owner and preventing
two noncanonical rows from being assigned the same unique email.
- Around line 36-37: Update the migration’s unique-index creation for
RefundRequest, Certificate, and all other target tables to use CREATE UNIQUE
INDEX CONCURRENTLY, and configure the migration step as non-transactional as
required by the database. Add duplicate preflight checks before each concurrent
index creation so existing duplicate values are detected before the index build;
preserve the intended unique constraints.
In `@src/app/`(public)/checkout/complete/page.tsx:
- Around line 110-123: Update ClaimCard to display a masked version of email in
the unauthenticated confirmation text, preserving the original email value for
claim-email delivery. Mask the local part while retaining the domain, such as
m***`@example.com`, and use the masked value only in the rendered <strong>
element.
In `@src/app/actions/__tests__/auth-actions.test.ts`:
- Around line 120-127: Update the updateMany assertion in the auth action test
to verify the guarded where predicate includes both passwordHash and
claimTokenHash, in addition to id and claimTokenExpiresAt. Preserve the existing
expiry-date predicate and data assertions while ensuring removal of either
security field causes the test to fail.
- Line 118: In the success branch of the auth action test, remove the
unnecessary any cast and access result.data.userId directly after result.success
narrows the result union; preserve the existing expectation for the guest user
ID.
In `@src/app/actions/progress.ts`:
- Around line 183-202: Before the createQuizAttempt call in the submission
action, validate lesson.quiz.isPublished and reject unpublished quizzes
immediately. Ensure this guard runs before recording the attempt or entering the
passed completion and XP-awarding flow, while preserving existing behavior for
published quizzes.
In `@src/app/actions/tools.ts`:
- Around line 150-174: The transaction in the submission flow must use the
result of the `toolSession.updateMany` call and only create the XP ledger entry
and increment user XP when its count is exactly 1. When the transition loses,
reload the persisted terminal session state, re-grade it, and use that result
for the response and badge evaluation instead of reporting the attempted pass.
In `@src/lib/certificates.ts`:
- Around line 133-136: Replace the em dash in the comment near the concurrency
backstop in src/lib/certificates.ts lines 133-136 with a period or comma. Apply
the same punctuation-only change to the corresponding comment in
src/lib/tracing.ts lines 122-123; make no other changes.
In `@src/lib/email.tsx`:
- Around line 190-196: The account-claim email delivery must expose failures and
be awaited by enrollment. In src/lib/email.tsx lines 190-196, update the
account-claim send flow to propagate failures or return an explicit delivery
result; in src/lib/enrollment.ts lines 359-367, await that result and persist
delivery through an outbox or retry job after the transaction commits, ensuring
failed delivery remains observable and retryable.
In `@src/lib/enrollment.ts`:
- Around line 449-489: The refund webhook handling around
alreadyRefundedAmountPhp must accumulate provider-side partial refunds instead
of replacing the stored total with the current amount. When no processed
RefundRequest exists, combine the existing payment.refundAmountPhp with the new
event amount, while preserving idempotency for repeated webhook refund IDs;
alternatively persist each provider refund by unique ID before summing. Update
fullyRefunded evaluation accordingly and add a regression test covering two
distinct partial-refund events that trigger full-refund revocation.
---
Outside diff comments:
In `@src/app/`(public)/auth/signup/SignUpForm.tsx:
- Around line 57-66: Update the email Input in SignUpForm so it is read-only
whenever claimToken is present, preventing edits to the prefilled claim email
while preserving normal editing when no claim token exists.
In `@src/app/actions/refunds.ts`:
- Around line 237-254: The refundPayment error path currently marks every
PayMongo exception as FAILED, including ambiguous transport errors. Preserve
APPROVED for timeouts, connection resets, and other unknown outcomes, retrying
with an Idempotency-Key derived from request.id; transition to FAILED only when
PayMongo status and error details identify an explicit 4xx rejection, while
retaining the existing failure metadata and revalidation for confirmed
rejections.
In `@src/app/actions/tools.ts`:
- Around line 95-108: Make the session freeze check atomic in the save flow
around the existing status validation and db.toolSession.update call. Replace
the separate status check and unconditional update with updateMany guarded by
both the session id and status: 'IN_PROGRESS', then reject with the existing
submitted-session error when the result count is zero; preserve the current
state and timeSpentSeconds values.
---
Nitpick comments:
In `@docs/AUDIT-REMEDIATION-2026-07-17.md`:
- Around line 3-6: Rewrite the opening of the audit document with a concrete,
plain-language summary of what this branch fixes instead of “Second-pass deep
audit response,” and expand “CI” to “continuous integration” at its first use.
Keep the scope and verification limitations clear while avoiding generic or
jargon-heavy wording.
In `@src/lib/__tests__/xp.test.ts`:
- Around line 1-12: Move the XP test suite from src/lib/__tests__/xp.test.ts to
src/lib/xp.test.ts, keeping its awardXpOnce coverage and setup intact. Move or
merge the affected authentication cases from
src/app/actions/__tests__/auth-actions.test.ts lines 96-130 into
src/app/actions/auth.test.ts, preserving their existing behavior and assertions.
In `@src/lib/claim-token.ts`:
- Line 21: Replace each em dash with a period or comma at the specified sites:
src/lib/claim-token.ts lines 21-21; src/lib/email.tsx lines 175-178 and 256-258;
src/lib/enrollment.ts lines 359-361 and 455-460;
src/app/(public)/checkout/complete/page.tsx lines 66-69 and 119-123;
src/app/actions/auth.ts lines 44-49; and src/lib/refunds.ts lines 75-81.
Preserve the existing meaning of comments and customer-facing copy.
In `@src/lib/enrollment.ts`:
- Around line 151-162: Update the placeholder passwordHash assignment in the
enrollment user creation flow to build the value with the exported
PLACEHOLDER_PASSWORD_PREFIX constant instead of hardcoding "placeholder_";
preserve the existing randomUUID suffix and claim-token behavior.
In `@src/lib/xp.ts`:
- Around line 8-13: Replace every em dash in the specified comments with
periods, commas, or parentheses, preserving the existing meaning. Update
src/lib/xp.ts lines 8-13 and 39, src/app/actions/progress.ts lines 97-100,
src/app/actions/tools.ts lines 95-99, 139-141, 176-178, and 232-235,
src/lib/__tests__/xp.test.ts lines 47-49,
src/app/(dashboard)/courses/[courseSlug]/lessons/[lessonSlug]/page.tsx lines
85-87, and src/app/actions/refunds.ts lines 224-227, 257-265, and 299.
🪄 Autofix (Beta)
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: cdd82441-2148-42c2-b0b2-58332833ebf2
📒 Files selected for processing (24)
docs/AUDIT-REMEDIATION-2026-07-17.mdprisma/migrations/20260717000000_audit_integrity_fixes/migration.sqlprisma/schema.prismasrc/app/(dashboard)/courses/[courseSlug]/lessons/[lessonSlug]/page.tsxsrc/app/(public)/auth/signup/SignUpForm.tsxsrc/app/(public)/auth/signup/page.tsxsrc/app/(public)/checkout/complete/page.tsxsrc/app/actions/__tests__/auth-actions.test.tssrc/app/actions/auth.tssrc/app/actions/checkout.tssrc/app/actions/progress.tssrc/app/actions/refunds.tssrc/app/actions/tools.tssrc/lib/__tests__/enrollment.test.tssrc/lib/__tests__/xp.test.tssrc/lib/certificates.tssrc/lib/claim-token.tssrc/lib/email.tsxsrc/lib/enrollment.tssrc/lib/prisma-errors.tssrc/lib/refunds.tssrc/lib/tracing.tssrc/lib/validation.tssrc/lib/xp.ts
| CREATE UNIQUE INDEX "RefundRequest_paymongoRefundId_key" | ||
| ON "RefundRequest" ("paymongoRefundId"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migration file context =="
wc -l prisma/migrations/20260717000000_audit_integrity_fixes/migration.sql
sed -n '1,140p' prisma/migrations/20260717000000_audit_integrity_fixes/migration.sql
echo
echo "== search for migration transaction control / concurrent index usage =="
rg -n "CONCURRENTLY|--\s*Create|migration.sql|BEGIN;|COMMIT;|prisma migrate|sql.*transaction|ALTER INDEX" prisma . -g '!node_modules' -g '!dist' -g '!build' || true
echo
echo "== list migration folders around this one =="
find prisma/migrations -maxdepth 1 -mindepth 1 -type d | sort | tail -n 10Repository: projectamazonph/amph-v2
Length of output: 31521
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Prisma schema for the affected tables and existing index definitions.
rg -n 'model (RefundRequest|Certificate)|@@index|@@unique|`@unique`|paymongoRefundId|certificate' prisma/schema.prisma prisma/migrations -g '!node_modules' -g '!dist' -g '!build' || trueRepository: projectamazonph/amph-v2
Length of output: 7631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the migration file contains any explicit lock-safe strategy or data backfill steps.
python3 - <<'PY'
from pathlib import Path
p = Path('prisma/migrations/20260717000000_audit_integrity_fixes/migration.sql')
text = p.read_text()
for needle in [
'CREATE UNIQUE INDEX',
'CONCURRENTLY',
'DELETE FROM',
'SELECT',
'ALTER TABLE',
'LOCK TABLE',
]:
print(f'{needle}:', text.count(needle))
PYRepository: projectamazonph/amph-v2
Length of output: 254
🌐 Web query:
Prisma Migrate PostgreSQL CREATE INDEX CONCURRENTLY transaction support
💡 Result:
As of Prisma 7.4.0, released in February 2026, Prisma Migrate includes native support for running CREATE INDEX CONCURRENTLY commands in PostgreSQL migrations [1][2]. Prior to this update, Prisma would often wrap migrations in transaction blocks, which caused the PostgreSQL error: ERROR: CREATE INDEX CONCURRENTLY cannot run inside a transaction block [2][3]. Users previously had to rely on workarounds, such as splitting migrations into single-statement files to prevent Prisma from triggering an automatic transaction block [4][2][3]. With the release of version 7.4.0, Prisma engines were updated to intelligently handle script execution by splitting migration scripts into individual statements [1][5]. This change ensures that commands incompatible with transaction blocks, such as CREATE INDEX CONCURRENTLY, are executed successfully without being wrapped in an implicit transaction [1][5]. This resolves the long-standing limitation where Prisma's automatic transaction handling conflicted with PostgreSQL's requirements for concurrent index creation [4][2].
Citations:
- 1: feat: support CREATE INDEX CONCURRENTLY in migrations prisma/prisma-engines#5767
- 2: Support
CREATE INDEX CONCURRENTLY(PostgreSQL) prisma/prisma#14456 - 3: Disable transactions for a single migration prisma/prisma#10601
- 4: Confusing transaction semantics in Postgres migrations prisma/prisma#22922
- 5: prisma/prisma-engines@57b675f
🌐 Web query:
PostgreSQL CREATE UNIQUE INDEX blocking writes without CONCURRENTLY documentation
💡 Result:
In PostgreSQL, running a standard CREATE UNIQUE INDEX command without the CONCURRENTLY option will block all write operations (INSERT, UPDATE, DELETE) on the target table until the index creation is complete [1][2][3]. This occurs because the standard CREATE INDEX command acquires a SHARE lock on the table [1][4], which permits concurrent SELECT queries but conflicts with any DDL or data modification operations [1][4]. To avoid blocking writes, you must use the CONCURRENTLY option [5][6][2]. The CONCURRENTLY parameter changes the locking behavior by acquiring a SHARE UPDATE EXCLUSIVE lock instead of a SHARE lock [1]. This lock mode is specifically designed to allow concurrent table data modifications (INSERT, UPDATE, DELETE) while preventing conflicting DDL operations [1]. Key considerations when choosing between these methods include: - Blocking Behavior: Standard CREATE INDEX blocks all writes [2][3]. CREATE INDEX CONCURRENTLY does not block writes [2][3]. - Performance: CREATE INDEX CONCURRENTLY requires more total work, including multiple table scans, and typically takes significantly longer to complete than a standard index build [5][2][3]. - Resource Usage: Because it takes longer, the CONCURRENTLY method imposes a more sustained load on CPU and I/O [5][2]. - Restrictions: CREATE INDEX CONCURRENTLY cannot be performed inside a transaction block [2] and only one concurrent index build is permitted on a table at a time [1][2]. By leveraging CONCURRENTLY, you can build unique indexes on production tables without disrupting ongoing write activity, albeit at the cost of a longer, more resource-intensive build process [5][2][3].
Citations:
- 1: https://www.bytebase.com/blog/postgres-create-index-concurrently/
- 2: https://www.postgresql.org/docs/17/sql-createindex.html
- 3: https://dba.stackexchange.com/questions/333044/what-is-the-difference-between-create-index-and-create-index-concurrently-in-pos
- 4: https://www.postgresql.org/docs/current/explicit-locking.html
- 5: https://www.postgresql.org/docs/18/sql-createindex.html
- 6: https://www.postgresql.org/docs/current/sql-createindex.html
Build these unique indexes without blocking writes.
CREATE UNIQUE INDEX on RefundRequest, Certificate, and the other target table will block inserts/updates/deletes while the index builds. Use CREATE UNIQUE INDEX CONCURRENTLY in a non-transactional migration step, and preflight duplicates first.
🧰 Tools
🪛 Squawk (2.59.0)
[warning] 36-37: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 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 `@prisma/migrations/20260717000000_audit_integrity_fixes/migration.sql` around
lines 36 - 37, Update the migration’s unique-index creation for RefundRequest,
Certificate, and all other target tables to use CREATE UNIQUE INDEX
CONCURRENTLY, and configure the migration step as non-transactional as required
by the database. Add duplicate preflight checks before each concurrent index
creation so existing duplicate values are detected before the index build;
preserve the intended unique constraints.
Source: Linters/SAST tools
| UPDATE "User" u | ||
| SET email = lower(btrim(u.email)) | ||
| WHERE u.email <> lower(btrim(u.email)) | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM "User" o | ||
| WHERE o.id <> u.id | ||
| AND o.email = lower(btrim(u.email)) | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle collisions between two noncanonical emails.
Two rows such as User@x.com and user@x.com both pass this NOT EXISTS check because neither currently equals the canonical value. The update then attempts to assign the same unique email to both and fails the migration.
Proposed collision-safe backfill
-UPDATE "User" u
-SET email = lower(btrim(u.email))
-WHERE u.email <> lower(btrim(u.email))
- AND NOT EXISTS (
- SELECT 1 FROM "User" o
- WHERE o.id <> u.id
- AND o.email = lower(btrim(u.email))
- );
+WITH canonicalized AS (
+ SELECT
+ id,
+ lower(btrim(email)) AS canonical_email,
+ count(*) OVER (
+ PARTITION BY lower(btrim(email))
+ ) AS collision_count
+ FROM "User"
+)
+UPDATE "User" u
+SET email = c.canonical_email
+FROM canonicalized c
+WHERE u.id = c.id
+ AND u.email <> c.canonical_email
+ AND c.collision_count = 1;📝 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.
| UPDATE "User" u | |
| SET email = lower(btrim(u.email)) | |
| WHERE u.email <> lower(btrim(u.email)) | |
| AND NOT EXISTS ( | |
| SELECT 1 FROM "User" o | |
| WHERE o.id <> u.id | |
| AND o.email = lower(btrim(u.email)) | |
| ); | |
| WITH canonicalized AS ( | |
| SELECT | |
| id, | |
| lower(btrim(email)) AS canonical_email, | |
| count(*) OVER ( | |
| PARTITION BY lower(btrim(email)) | |
| ) AS collision_count | |
| FROM "User" | |
| ) | |
| UPDATE "User" u | |
| SET email = c.canonical_email | |
| FROM canonicalized c | |
| WHERE u.id = c.id | |
| AND u.email <> c.canonical_email | |
| AND c.collision_count = 1; |
🤖 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 `@prisma/migrations/20260717000000_audit_integrity_fixes/migration.sql` around
lines 61 - 68, Update the email normalization UPDATE so it only canonicalizes a
row when no other user already has, or will normalize to, the same email value.
Adjust the collision check around the User migration query to compare each other
row’s canonicalized email (not only o.email) against lower(btrim(u.email)),
preserving one existing canonical owner and preventing two noncanonical rows
from being assigned the same unique email.
| // H7: the partial unique index (one ACTIVE certificate per user+course) is | ||
| // the concurrency backstop. Two simultaneous issues both pass the findFirst | ||
| // above; the losing insert hits P2002, and we return the winner's row as if | ||
| // it already existed — never two active certificates. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove em dashes from source comments. Both changed TypeScript comments violate the source punctuation rule.
src/lib/certificates.ts#L133-L136: replace the em dash with a period or comma.src/lib/tracing.ts#L122-L123: replace the em dash with a period or comma.
As per coding guidelines, “Do not use emojis in code or commit messages, and do not use em-dashes.”
📍 Affects 2 files
src/lib/certificates.ts#L133-L136(this comment)src/lib/tracing.ts#L122-L123
🤖 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 `@src/lib/certificates.ts` around lines 133 - 136, Replace the em dash in the
comment near the concurrency backstop in src/lib/certificates.ts lines 133-136
with a period or comma. Apply the same punctuation-only change to the
corresponding comment in src/lib/tracing.ts lines 122-123; make no other
changes.
Source: Coding guidelines
| await sendEmail({ | ||
| to, | ||
| subject: `Claim your ${BRAND_NAME} account`, | ||
| react: ( | ||
| <AccountClaimEmail claimUrl={claimUrl.toString()} tierName={tierName} /> | ||
| ), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make account-claim delivery observable and retryable.
This email is the only delivery of the claim credential, but failures are swallowed and the promise is not awaited. A serverless invocation can finish before delivery, leaving a paid buyer unable to claim the account.
src/lib/email.tsx#L190-L196: propagate delivery failures, or return an explicit delivery result.src/lib/enrollment.ts#L359-L367: await delivery and preferably persist it through an outbox or retry job after the transaction commits.
📍 Affects 2 files
src/lib/email.tsx#L190-L196(this comment)src/lib/enrollment.ts#L359-L367
🤖 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 `@src/lib/email.tsx` around lines 190 - 196, The account-claim email delivery
must expose failures and be awaited by enrollment. In src/lib/email.tsx lines
190-196, update the account-claim send flow to propagate failures or return an
explicit delivery result; in src/lib/enrollment.ts lines 359-367, await that
result and persist delivery through an outbox or retry job after the transaction
commits, ensuring failed delivery remains observable and retryable.
| const payment = await tx.payment.findUnique({ | ||
| where: { paymongoPaymentId: paymentIdPm }, | ||
| select: { id: true }, | ||
| select: { id: true, amountPhp: true }, | ||
| }); | ||
| if (!payment) return; | ||
|
|
||
| // C8: cumulative refunded amount is derived from the PROCESSED refund | ||
| // requests — the single source of truth. If the admin path has already | ||
| // recorded this refund, that sum already includes it and we must NOT add | ||
| // the webhook `amount` on top. When no processed request exists yet (a | ||
| // refund initiated outside our flow, or the admin DB write is still | ||
| // pending), fall back to the event amount without double-adding. | ||
| const processedSum = await alreadyRefundedAmountPhp(payment.id, tx); | ||
| const refundedTotal = processedSum > 0 ? processedSum : amount; | ||
| const fullyRefunded = refundedTotal >= payment.amountPhp; | ||
|
|
||
| await tx.payment.update({ | ||
| where: { id: payment.id }, | ||
| data: { | ||
| status: PaymentStatus.REFUNDED, | ||
| status: fullyRefunded ? PaymentStatus.REFUNDED : PaymentStatus.PARTIALLY_REFUNDED, | ||
| refundedAt: new Date(), | ||
| refundAmountPhp: amount, | ||
| refundAmountPhp: refundedTotal, | ||
| }, | ||
| }); | ||
| // Access enrollment via relation | ||
| const enrollment = await tx.enrollment.findFirst({ | ||
| where: { payment: { id: payment.id } }, | ||
| }); | ||
| if (enrollment) { | ||
| await tx.enrollment.update({ | ||
| where: { id: enrollment.id }, | ||
| data: { | ||
| status: 'REFUNDED', | ||
| cancelledAt: new Date(), | ||
| cancellationReason: 'Refund processed', | ||
| }, | ||
|
|
||
| // Only revoke access on a FULL refund. A partial refund leaves the | ||
| // enrollment active. | ||
| if (fullyRefunded) { | ||
| const enrollment = await tx.enrollment.findFirst({ | ||
| where: { payment: { id: payment.id } }, | ||
| }); | ||
| if (enrollment) { | ||
| await tx.enrollment.update({ | ||
| where: { id: enrollment.id }, | ||
| data: { | ||
| status: 'REFUNDED', | ||
| cancelledAt: new Date(), | ||
| cancellationReason: 'Refund processed', | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Accumulate provider-side partial refunds instead of replacing the total.
When no processed RefundRequest exists, every webhook sets refundAmountPhp to only the current event amount. Two external partial refunds therefore lose the first amount and may never trigger full-refund revocation. Include the payment's existing refunded amount, or persist each provider refund by its unique refund ID before summing it. Add a regression test covering two distinct partial-refund events.
🤖 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 `@src/lib/enrollment.ts` around lines 449 - 489, The refund webhook handling
around alreadyRefundedAmountPhp must accumulate provider-side partial refunds
instead of replacing the stored total with the current amount. When no processed
RefundRequest exists, combine the existing payment.refundAmountPhp with the new
event amount, while preserving idempotency for repeated webhook refund IDs;
alternatively persist each provider refund by unique ID before summing. Update
fullyRefunded evaluation accordingly and add a regression test covering two
distinct partial-refund events that trigger full-refund revocation.
- tools submit: award XP only when this request wins the IN_PROGRESS -> terminal transition (count === 1); on a lost race, reconcile from the recorded terminal state instead of reporting an unpersisted pass. - tools save: fold the freeze check into a status-guarded updateMany so the check and write are atomic. - progress submitQuiz: reject unpublished (draft) quizzes before recording an attempt or awarding completion/XP. - refund approve: only an explicit PayMongo 4xx marks the request FAILED; ambiguous transport/5xx errors stay APPROVED for reconciliation so a possibly-successful refund is never misreported or re-issued (C9). - signup form: lock the email field when a claim token is present so a buyer can't create a second account and orphan the paid enrollment. - checkout complete: mask the buyer email on the unauthenticated page. - enrollment: build placeholder hashes from PLACEHOLDER_PASSWORD_PREFIX. - tests: assert every guard predicate on the claim consume; drop an any cast; add atomic save-freeze regression tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoQ1YGTKVSky148W8EAr9Z
|
Thanks for the review. Triaged all findings against the actual code. Fixes are in Fixed
Deferred (with reasons; tracked in
Not applicable to this repo
Generated by Claude Code |
|
Superseded by #46, which keeps this PR's DB-enforced idempotency (XpLedger, partial unique indexes, atomic claims) and its quiz-gate / tool-session fixes, but adds the Generated by Claude Code |
…st-review hardening (#46) Reconciles the two competing audit-remediation PRs (#33, #34) into one shippable change, fixes the broken Source-based checkout flow that main never handled, and adds post-review hardening: webhook idempotency + amount/currency validation, certificate P2002 race handling, refund audit logging, requireAdmin fail-closed, email canonicalization + collision safety, and the CI seed fix. Supersedes #33 and #34.
2 participants
Foundation for the 2026-07-17 audit remediation (integrity + security
findings that apply to main):
claiming (C5/C6).
one active certificate per (user, course) (H7).
claim-token mint/hash.
See docs/AUDIT-REMEDIATION-2026-07-17.md for scope and deferred items.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01GoQ1YGTKVSky148W8EAr9Z
Summary by CodeRabbit
New Features
Bug Fixes
Tests