Reconcile PR #33 + #34 audit remediation (fixes broken checkout) + post-review hardening#46
Merged
Merged
Conversation
…ation Both PR #33 and PR #34 independently added User.claimTokenHash / claimTokenExpiresAt via separate migrations that would collide (duplicate-column error) if stacked. This adds ONE new migration (20260718000000_audit_integrity_fixes) against current main's schema, based on pr34's superset migration content (XpLedger table, partial unique indexes for one-active-refund-per-payment and one-active-certificate-per-user-course, unique RefundRequest.paymongoRefundId, canonical-email backfill) plus pr33's User_claimTokenHash_idx index that pr34 was missing. Verified by applying both migrations to a fresh local Postgres 16 instance via `prisma migrate deploy` and confirming `prisma migrate diff` between the resulting DB and schema.prisma is empty (no drift). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
…handled, and fix guest-checkout claim-token delivery Top-priority fix: checkout.ts creates a PayMongo Source (the SDK has no checkoutSessions resource — see paymongo.ts), but the webhook route only handled checkout_session.payment.* events and explicitly dropped source.chargeable / payment.paid — the events a Source-based purchase actually fires. No real purchase could complete on main. This brings handleSourceChargeable/handlePaymentPaid (source: pr33) into enrollment.ts and wires them into the webhook route's switch statement, adapted to current main's handler signatures and enum names. Fixes pr33's dead guest-claim-token code along the way: pr33 generated a raw claim token in findOrCreateUserByEmail but every call site (including the new Source-flow handlers) discarded it and only logged that "a token was emailed" — no email was ever sent. This wires the real flow end to end: findOrCreateUserByEmail (now using pr34's claim-token.ts mint/hash helpers, canonicalized email, PLACEHOLDER_PASSWORD_PREFIX) returns rawClaimToken -> both handleCheckoutPaid and the new Source-flow handlers thread it through -> sendAccountClaimEmail (pr34, new) sends it after the DB transaction commits -> checkout/complete's ClaimCard (pr34, masked email) tells the guest to check their inbox instead of redirecting them to a tokenless signup that would now be rejected -> signup page/form (claim query param + hidden input, readOnly email) and the atomic updateMany-based token consumption in actions/auth.ts (pr34 — guards on id + placeholder marker + token hash + expiry in one UPDATE, closing the double-claim race pr33's findUnique-then-update version had) complete the loop. Also layers pr33's H1 fix onto checkout.ts: the CheckoutSession is now created before the PayMongo Source so its id can be embedded as `checkout_id` on the redirect URL, letting /checkout/complete reliably find the session. Deviation from pr33: its version used a literal '__pending__' placeholder for paymongoSourceId (which is @unique and nullable) while creating the session, which would collide under two concurrent checkouts; this leaves the field unset instead and updates it once the real source id exists. Also fixes a real bug found while porting: pr33's Source-flow Payment.create call omitted the required `netAmountPhp` field entirely, which would have failed at the Prisma layer. handlePaymentRefunded's cumulative-refund math now derives from alreadyRefundedAmountPhp (pr34, extended here to accept a transaction client) instead of a locally-tracked sum, so the admin approval path and the webhook can never double-count the same refund. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
…ency
progress.ts: XP awards now go through XpLedger-backed awardXpOnce
(pr34, new src/lib/xp.ts) — the ledger's unique (userId, eventKey) index
makes every award exactly-once under concurrency, replacing a plain
xp: {increment} that could double-fire on a retry or a racing request.
Also brings pr34's H1 quiz-gate fix: markLessonCompleteAction now rejects
manual completion when the lesson has a published quiz, closing a path
where a student could get credit (and certificate eligibility) without
passing it. pr33 has neither fix.
tools.ts: submitToolSession's session-transition (IN_PROGRESS -> GRADED/
SUBMITTED) is now a single atomic updateMany claim — only the winner of
a race writes the grade and XP; everyone else reconciles from the
recorded terminal state via awardXpOnce's own idempotency, so a retry or
concurrent double-submit can't double-grant XP or clobber a graded
session's stored state. saveToolSession also gets an atomic
IN_PROGRESS-only guard (H3) so a late autosave can't overwrite an
already-graded session — pr33 only fixes the submit race, not this one.
refunds.ts: createRefundRequestAction now relies on the partial unique
index added in the schema migration (one active refund per payment) as
the concurrency backstop for its pre-check, catching P2002 instead of
pr33's findFirst-then-create inside a plain transaction (Read Committed
doesn't actually close that race). approveRefundAction's error handling
(C9) now distinguishes an explicit 4xx rejection from PayMongo (safe to
mark FAILED) from an ambiguous outcome — timeout, 5xx, connection reset
— which is left APPROVED (not re-approvable, not FAILED) for the
payment.refunded webhook to reconcile; pr33's version unconditionally
marks FAILED on any error, which risks a double-refund if the provider
call actually succeeded. A DB failure after a successful PayMongo refund
(state 2) now reports success with a PROCESSING status instead of
falsely telling the admin the refund failed.
Brings over pr34's src/lib/prisma-errors.ts (isUniqueConstraintError)
used by all three files above.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
- New src/lib/redirect-url.ts: validateRedirectUrl() rejects javascript:, data:, //, backslash, control chars, and encoded-scheme redirect targets. Wired into the sign-in page/form's `redirect` query param (C3) — closes an open-redirect / XSS vector that previously trusted the param verbatim. - src/lib/auth.ts requireAdmin(): now loads the user's role from the DB instead of trusting the JWT's role claim, which can be stale (e.g. an admin demoted after their token was issued keeps admin access for the rest of the cookie's lifetime otherwise). - prisma/seed.ts: removes the [email protected] / ChangeMe123! fallback — seeding now fails loudly when ADMIN_EMAIL/ADMIN_PASSWORD aren't set (always, not just in production), instead of silently creating a well-known admin account. - src/lib/db.ts: removes ProcessedWebhook from SOFT_DELETE_MODELS — it has no deletedAt column, so every read on that model was injecting a filter on a field that doesn't exist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
- enrollment.test.ts: merges pr33's new-handler test scaffolding with pr34's mock updates (discountCode.updateMany, refundRequest.findMany, sendAccountClaimEmail, canonical-email node:crypto mocks), then adds fresh coverage tailored to the actual merged implementation: handleSourceChargeable/handlePaymentPaid happy paths, currency/duplicate/ not-found guards, the H4 atomic discount-limit guard, and — the key regression guard for this whole change — that a NEW guest user created during the Source-flow handlers actually gets sendAccountClaimEmail called with the raw token, not just a log line. - xp.test.ts: pr34's awardXpOnce tests, unchanged (new file, no reconciliation needed). - auth-actions.test.ts / tool-actions.test.ts: pr34's additions for the atomic claim-token consumption and the atomic tool-session guards. - validation.test.ts: pr33's validateRedirectUrl test suite. - auth.test.ts: requireAdmin mock updates for the two-DB-query shape, plus a new test asserting a demoted user with a stale ADMIN JWT is still redirected once the DB read disagrees. Omits pr33's generateClaimToken/verifyClaimToken tests — those functions were superseded by pr34's claim-token.ts and never added to auth.ts. Also fixes a latent test-pollution bug found while writing the new handleCheckoutPaid discount-limit test: wireHappyPath() queues two mockResolvedValueOnce values for user.findUnique, but a test whose flow throws mid-transaction (before the post-commit lookup) only consumes the first — the second leaks into and corrupts whichever test runs next. Fixed by giving that test its own minimal, non-leaking mock setup. All 280 tests pass (25 suites). Coverage: 87.5% statements / 77.6% branches / 87.3% functions / 88.7% lines on src/lib — above the 70% gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
…cert races, refund audit log Addresses the ten findings from the post-reconciliation review pass. Payments (src/lib/enrollment.ts): - handleSourceChargeable: claim webhook idempotency BEFORE charging, run the PayMongo call OUTSIDE the DB transaction (no locks/pooled connection held across an HTTP round-trip), release the claim on charge failure so a transient error retries instead of losing the sale, and fire post-purchase emails after commit (they read via the top-level client and can't see a transaction's uncommitted rows). - H2 amount/currency check now rejects underpayment and a non-PHP/empty currency instead of warning-and-proceeding; shared assertPaymentMatchesCheckout covers both the source.chargeable and payment.paid paths. - payment.paid now forwards the real source type so non-GCash payments aren't mis-recorded (fallback is OTHER, never a silent GCASH). Checkout: - checkout.ts marks the CheckoutSession ERROR when Source creation fails so it isn't orphaned as a permanent PENDING row (no cron/TTL reclaims it). - checkout/complete resolves strictly by session id; drop the dead paymongoReference fallback that was being fed the internal returnUrl. Certificates (src/lib/certificates.ts): - issueCertificate catches the H7 unique-index P2002 and returns the winning row; the auto-issue page render no longer crashes to the error boundary on a lost race. Refunds (src/app/actions/refunds.ts): - approve/reject now write AuditLog entries at every state transition (AGENTS.md: every admin action logs, no exceptions), best-effort so a log failure never misreports a refund that moved money. Auth (src/lib/auth.ts): - requireAdmin fails closed when the authoritative role lookup returns null rather than falling back to the JWT's possibly-stale role. Progress (src/app/actions/progress.ts): - award XP before marking a lesson complete so any XP failure leaves the lesson cleanly retryable instead of "completed but no XP"; documents why a single shared transaction is incompatible with the ledger's P2002 exactly-once gate. Email backfill: - H6 migration now emits a WARNING for collision-skipped rows; add scripts/report-email-collisions.ts to list them for manual reconciliation. Tests: cert P2002 idempotency, requireAdmin null-role fail-closed, source underpayment rejection; enrollment tests updated for post-commit email timing. All: typecheck clean, lint 0 errors, 283 tests pass, coverage 88/86/78/87% (>70% gate), build green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
This was referenced Jul 19, 2026
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds guest account claiming, secure redirect validation, Source-based PayMongo fulfillment, idempotent XP awards, stronger refund and certificate constraints, normalized emails, database-backed admin authorization, and concurrency-safe progress and tool-session updates. ChangesAudit remediation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PayMongo
participant Webhook
participant Enrollment
participant Database
PayMongo->>Webhook: source.chargeable
Webhook->>Enrollment: handleSourceChargeable
Enrollment->>PayMongo: create charge
PayMongo->>Webhook: payment.paid
Webhook->>Enrollment: handlePaymentPaid
Enrollment->>Database: create payment and enrollment
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 6
🧹 Nitpick comments (4)
src/lib/xp.ts (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace em-dashes in comments with periods, commas, or parentheses. The changed comments use em-dashes, which the repo style rule forbids in
.tsfiles. Root cause is the same across files; apply the same substitution everywhere.
src/lib/xp.ts#L39-L39: fix the em-dash on L39 ("insert first — its unique index") and the header comment on L12.src/app/actions/progress.ts#L99-L99: fix the em-dashes on L99 and L108 in themarkLessonCompleteActioncomment.src/app/actions/tools.ts#L143-L143: fix the em-dashes on L143 ("Grade BEFORE any write —") and L269 ("No database writes —").src/lib/certificates.ts#L117-L117: fix the em-dash on L117 ("fail with P2002 —").src/lib/__tests__/auth.test.ts#L229-L229: fix the em-dash on L229 ("DB twice —").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` at line 39, Replace all em-dashes in the affected comments with periods, commas, or parentheses, without changing code behavior: update the header and ledger-insert comments in src/lib/xp.ts lines 12 and 39; the markLessonCompleteAction comments in src/app/actions/progress.ts lines 99 and 108; the comments in src/app/actions/tools.ts lines 143 and 269; the P2002 comment in src/lib/certificates.ts line 117; and the database-call comment in src/lib/__tests__/auth.test.ts line 229.Source: Coding guidelines
src/lib/redirect-url.ts (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace em-dashes in code comments (coding-guideline violation). The newly added comments across these TypeScript files use em-dashes, which the guideline prohibits; replace them with periods, commas, or parentheses.
src/lib/redirect-url.ts#L13-L14: replace the em-dash in the control-character comment.src/app/actions/checkout.ts#L169-L172: replace em-dashes in the "create the CheckoutSession FIRST" comment block.src/app/actions/refunds.ts#L243-L246: replace em-dashes in the State 1 provider-call comment (and the other new comment blocks in this file).prisma/seed.ts#L37-L40: replace the em-dash in the C5 fail-loud comment.src/lib/db.ts#L46-L48: replace the em-dash in the ProcessedWebhook comment.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/redirect-url.ts` around lines 13 - 14, Replace every em dash in the newly added comments with periods, commas, or parentheses. Update the comments at src/lib/redirect-url.ts lines 13-14, src/app/actions/checkout.ts lines 169-172, src/app/actions/refunds.ts lines 243-246 and other new comment blocks in that file, prisma/seed.ts lines 37-40, and src/lib/db.ts lines 46-48; no code behavior changes are needed.Source: Coding guidelines
prisma/migrations/20260718000000_audit_integrity_fixes/migration.sql (1)
44-61: 🚀 Performance & Scalability | 🔵 TrivialDeploy-time locking and pre-existing duplicate risk on the new indexes.
Two operational points before applying this to a populated database:
- The partial unique indexes (
RefundRequest_active_per_payment_key,Certificate_active_per_user_course_key) will abort the migration if any pre-existing rows already violate them (e.g. two ACTIVE certificates for the same user/course). Worth pre-scanning for duplicates before deploy.- Squawk flags these
CREATE INDEXstatements (and theUser_claimTokenHash_idx) as blocking writes on existing tables.CREATE INDEX CONCURRENTLYavoids that but cannot run inside Prisma's transactional migration wrapper, so it would need a separate non-transactional step. Combined with the full-tableUPDATE "User"backfill, expect write locks proportional to table size during deploy.🤖 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/20260718000000_audit_integrity_fixes/migration.sql` around lines 44 - 61, Before creating RefundRequest_active_per_payment_key and Certificate_active_per_user_course_key, add a pre-deployment duplicate check or cleanup step for existing violating rows, and make the migration fail with actionable diagnostics if duplicates remain. Review User_claimTokenHash_idx and the new index creation strategy to avoid blocking writes: use a non-transactional migration path with concurrent index creation where supported, and account for locking from the full-table User backfill.Source: Linters/SAST tools
src/lib/validation.ts (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
z.email()for email validation.The trim/lowercase order is fine, but
z.string().email()is deprecated in Zod 4. Switch to the standalonez.email()and keep trimming/lowercasing in apipeorpreprocessstep if canonicalization must stay.🤖 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/validation.ts` around lines 29 - 30, Update canonicalEmail to use Zod 4’s standalone z.email() instead of the deprecated z.string().email() method, while preserving the existing trim and lowercase canonicalization through an appropriate pipe or preprocess step.
🤖 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/seed.ts`:
- Around line 37-57: The seed completion log should stop exposing credentials
and referencing removed defaults. Update the completion logging near the seed
function’s final output to omit the admin password entirely and remove the
fallback email value, logging only the configured admin email or other
non-sensitive status information.
In `@src/app/`(dashboard)/courses/[courseSlug]/certificate/page.tsx:
- Around line 81-82: Update the catch handling auto-issuance in the certificate
page to log the caught error through the structured logger from
src/lib/logger.ts, including the relevant user and course IDs, before setting
verificationHash to null and preserving the existing fallback behavior.
- Around line 77-83: Add a regression test for the certificate page fallback
around issueCertificate: mock issueCertificate to reject when verificationHash
is absent and completion.isComplete is true, then verify the page renders the
retry state without certificate links. Keep the test focused on this failure
path and preserve the existing successful issuance behavior.
In `@src/app/`(public)/checkout/complete/page.tsx:
- Line 55: Update the returnUrl assignment in the checkout completion page to
ensure missing or invalid rawReturnUrl values resolve to undefined rather than
validateRedirectUrl’s default '/'. Pass an explicit undefined-producing fallback
to validateRedirectUrl, preserving valid redirect URLs so FailedCard’s returnUrl
?? '/pricing' fallback sends failed checkouts to pricing.
In `@src/app/actions/refunds.ts`:
- Around line 301-305: Update the error message in the ambiguous-outcome return
of the refund action to state that the request was left for automatic
reconciliation, rather than saying it was left pending. Preserve the existing
unknown-status and do-not-retry guidance.
In `@src/lib/auth.ts`:
- Around line 202-212: Replace the em dashes in the authorization comments near
the database role lookup in src/lib/auth.ts lines 202-212 with commas or
parentheses, preserving the comment meaning. Also replace the em dash in the
resilience comment in
src/app/(dashboard)/courses/[courseSlug]/certificate/page.tsx lines 73-75 with a
comma or period; no other changes are needed.
---
Nitpick comments:
In `@prisma/migrations/20260718000000_audit_integrity_fixes/migration.sql`:
- Around line 44-61: Before creating RefundRequest_active_per_payment_key and
Certificate_active_per_user_course_key, add a pre-deployment duplicate check or
cleanup step for existing violating rows, and make the migration fail with
actionable diagnostics if duplicates remain. Review User_claimTokenHash_idx and
the new index creation strategy to avoid blocking writes: use a
non-transactional migration path with concurrent index creation where supported,
and account for locking from the full-table User backfill.
In `@src/lib/redirect-url.ts`:
- Around line 13-14: Replace every em dash in the newly added comments with
periods, commas, or parentheses. Update the comments at src/lib/redirect-url.ts
lines 13-14, src/app/actions/checkout.ts lines 169-172,
src/app/actions/refunds.ts lines 243-246 and other new comment blocks in that
file, prisma/seed.ts lines 37-40, and src/lib/db.ts lines 46-48; no code
behavior changes are needed.
In `@src/lib/validation.ts`:
- Around line 29-30: Update canonicalEmail to use Zod 4’s standalone z.email()
instead of the deprecated z.string().email() method, while preserving the
existing trim and lowercase canonicalization through an appropriate pipe or
preprocess step.
In `@src/lib/xp.ts`:
- Line 39: Replace all em-dashes in the affected comments with periods, commas,
or parentheses, without changing code behavior: update the header and
ledger-insert comments in src/lib/xp.ts lines 12 and 39; the
markLessonCompleteAction comments in src/app/actions/progress.ts lines 99 and
108; the comments in src/app/actions/tools.ts lines 143 and 269; the P2002
comment in src/lib/certificates.ts line 117; and the database-call comment in
src/lib/__tests__/auth.test.ts line 229.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bbd06281-1121-42d2-aa41-6848235ee7b1
📒 Files selected for processing (34)
prisma/migrations/20260718000000_audit_integrity_fixes/migration.sqlprisma/schema.prismaprisma/seed.tsscripts/report-email-collisions.tssrc/app/(dashboard)/courses/[courseSlug]/certificate/page.tsxsrc/app/(public)/auth/signin/SignInForm.tsxsrc/app/(public)/auth/signin/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/__tests__/tool-actions.test.tssrc/app/actions/auth.tssrc/app/actions/checkout.tssrc/app/actions/progress.tssrc/app/actions/refunds.tssrc/app/actions/tools.tssrc/app/api/paymongo/webhook/route.tssrc/lib/__tests__/auth.test.tssrc/lib/__tests__/certificates.test.tssrc/lib/__tests__/enrollment.test.tssrc/lib/__tests__/validation.test.tssrc/lib/__tests__/xp.test.tssrc/lib/auth.tssrc/lib/certificates.tssrc/lib/claim-token.tssrc/lib/db.tssrc/lib/email.tsxsrc/lib/enrollment.tssrc/lib/prisma-errors.tssrc/lib/redirect-url.tssrc/lib/refunds.tssrc/lib/validation.tssrc/lib/xp.ts
Comment on lines
+202
to
+212
| // H3: Load the authoritative role from the database — the JWT claim can be | ||
| // stale (e.g. an admin was demoted after the token was issued, but the | ||
| // cookie is still valid for the rest of its lifetime). | ||
| const dbUser = await db.user.findUnique({ | ||
| where: { id: user.id }, | ||
| select: { role: true }, | ||
| }); | ||
|
|
||
| // Fail closed: if the row is missing (soft-deleted between requireAuth's | ||
| // lookup and this one — a narrow TOCTOU window), deny rather than fall back | ||
| // to the JWT's possibly-stale role claim, which would defeat this check. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove em dashes from changed code comments.
src/lib/auth.ts#L202-L212: replace the em dashes in the authorization comments with commas or parentheses.src/app/(dashboard)/courses/[courseSlug]/certificate/page.tsx#L73-L75: replace the em dash in the resilience comment with a comma or period.
As per coding guidelines, “Do not use emojis in code or commit messages, and do not use em-dashes.”
📍 Affects 2 files
src/lib/auth.ts#L202-L212(this comment)src/app/(dashboard)/courses/[courseSlug]/certificate/page.tsx#L73-L75
🤖 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/auth.ts` around lines 202 - 212, Replace the em dashes in the
authorization comments near the database role lookup in src/lib/auth.ts lines
202-212 with commas or parentheses, preserving the comment meaning. Also replace
the em dash in the resilience comment in
src/app/(dashboard)/courses/[courseSlug]/certificate/page.tsx lines 73-75 with a
comma or period; no other changes are needed.
Source: Coding guidelines
- seed.ts: completion log no longer prints the admin password or the removed well-known [email protected]/ChangeMe123! defaults (dead ?? branches). - certificate page: log auto-issue failures via the structured logger (userId, courseId) instead of a silent catch. - checkout/complete: pass '' fallback to validateRedirectUrl so missing/invalid returnUrl becomes undefined, letting a failed checkout link back to /pricing instead of /. - refunds: ambiguous-outcome message now says "left for automatic reconciliation" (the request stays APPROVED, not PENDING). - Remove em-dashes from comments added by this branch (repo guideline). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/report-email-collisions.ts (1)
29-41: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDetect collisions among all canonical-equivalent emails.
The current
EXISTScondition only finds a noncanonical email when another row already contains the exact canonical value. It misses collisions such asUser@Example.comandUSER@example.com, where both rows are noncanonical butlower(btrim(email))is identical. The script would report no collisions, while the canonicalization migration could fail on the unique constraint.Replace the exact-match lookup with a grouped canonical-value query using
HAVING COUNT(*) > 1, and return every member of each colliding group.🤖 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 `@scripts/report-email-collisions.ts` around lines 29 - 41, Update the collision query in scripts/report-email-collisions.ts to group all users by lower(btrim(u.email)), filter groups with HAVING COUNT(*) > 1, and return every user in those colliding canonical-value groups. Remove the current noncanonical filter and exact-value EXISTS lookup so cases like differently cased noncanonical emails are included, while preserving the existing canonical ordering and no-collisions output.prisma/seed.ts (1)
42-57: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCanonicalize the seeded admin email before storing it.
rawEmailis trimmed but not lowercased, while the application contract usestrim().toLowerCase(). A mixed-caseADMIN_EMAILcan therefore create a noncanonical admin row and fail to match canonicalized authentication lookups.Proposed fix
- const rawEmail = (process.env.ADMIN_EMAIL ?? '').trim(); + const rawEmail = (process.env.ADMIN_EMAIL ?? '').trim().toLowerCase();🤖 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/seed.ts` around lines 42 - 57, Canonicalize the seeded admin email before persistence by applying lowercase normalization to the trimmed ADMIN_EMAIL value. Update the email assignment in the seed flow around rawEmail so it uses the same trim().toLowerCase() contract as authentication lookups, while leaving password handling unchanged.
🤖 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.
Outside diff comments:
In `@prisma/seed.ts`:
- Around line 42-57: Canonicalize the seeded admin email before persistence by
applying lowercase normalization to the trimmed ADMIN_EMAIL value. Update the
email assignment in the seed flow around rawEmail so it uses the same
trim().toLowerCase() contract as authentication lookups, while leaving password
handling unchanged.
In `@scripts/report-email-collisions.ts`:
- Around line 29-41: Update the collision query in
scripts/report-email-collisions.ts to group all users by lower(btrim(u.email)),
filter groups with HAVING COUNT(*) > 1, and return every user in those colliding
canonical-value groups. Remove the current noncanonical filter and exact-value
EXISTS lookup so cases like differently cased noncanonical emails are included,
while preserving the existing canonical ordering and no-collisions output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 41c9dcc2-05f6-420a-9674-7f849c853417
📒 Files selected for processing (13)
prisma/migrations/20260718000000_audit_integrity_fixes/migration.sqlprisma/seed.tsscripts/report-email-collisions.tssrc/app/(dashboard)/courses/[courseSlug]/certificate/page.tsxsrc/app/(public)/checkout/complete/page.tsxsrc/app/actions/checkout.tssrc/app/actions/progress.tssrc/app/actions/refunds.tssrc/lib/__tests__/auth.test.tssrc/lib/__tests__/enrollment.test.tssrc/lib/auth.tssrc/lib/certificates.tssrc/lib/enrollment.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/app/(dashboard)/courses/[courseSlug]/certificate/page.tsx
- src/lib/auth.ts
- src/lib/certificates.ts
- src/app/actions/checkout.ts
- src/app/(public)/checkout/complete/page.tsx
- src/lib/tests/auth.test.ts
- prisma/migrations/20260718000000_audit_integrity_fixes/migration.sql
- src/app/actions/progress.ts
- src/app/actions/refunds.ts
- src/lib/tests/enrollment.test.ts
- src/lib/enrollment.ts
… safety CI (the actual failing checks): - E2E and Lighthouse jobs ran `tsx prisma/seed.ts` without ADMIN_EMAIL / ADMIN_PASSWORD, which the C5 hardening now requires (seed throws "ADMIN_EMAIL must not be empty" and aborts before seeding tiers/courses). Provide CI-only test admin credentials in both job env blocks. CodeRabbit Major findings: - seed.ts: canonicalize ADMIN_EMAIL with trim().toLowerCase() to match the app's sign-in lookup contract (a mixed-case env value otherwise seeds a row auth can't find). - H6 email backfill: the collision guard compared canonical-to-exact, so two rows that are BOTH non-canonical but share a canonical form (e.g. 'Foo@Bar.com' + 'FOO@bar.com') both passed the guard and both UPDATE to the same value in one MVCC-snapshotted statement, violating the unique email index and failing the migration. Compare canonical-to-canonical so every member of a colliding group is skipped. Same fix in the WARNING count query and in scripts/report-email-collisions.ts (grouped HAVING COUNT(*) > 1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
Owner
Author
|
Note on claim-token TTL: issue #33 specified 24h, but this PR intentionally uses a 7-day expiry (consistent across Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/report-email-collisions.ts`:
- Around line 51-52: Update the collision-report logging loop around rows and
its console.log call to mask both r.email and r.canonical by default, while
retaining enough information to identify the collision. Add an explicit opt-in
for raw reconciliation values and ensure raw emails are emitted only when that
access-controlled option is enabled; do not log full addresses in the default
path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f3b03b6a-40a1-49ed-8b0d-9a22497bb1e7
📒 Files selected for processing (4)
.github/workflows/ci.ymlprisma/migrations/20260718000000_audit_integrity_fixes/migration.sqlprisma/seed.tsscripts/report-email-collisions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- prisma/seed.ts
- prisma/migrations/20260718000000_audit_integrity_fixes/migration.sql
The email-collision report printed full addresses (PII) to stdout, which CI/cron/log-aggregation could retain. Mask the local part by default (keeping the opaque user id, which is the actionable key), and require an explicit SHOW_EMAILS=1 opt-in to print full addresses for reconciliation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
This was referenced Jul 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
PRs #33 and #34 both fix the same 2026-07-17 security/integrity audit, but they conflict on 11 of 12 shared files and each is individually non-shippable:
updateManyclaims) and fixes two things fix: AMPH-v2 full codebase audit remediation (C1-C7, H1-H7) #33 misses (quiz-gate bypass, tool-session overwrite) — but never wires thesource.chargeable/payment.paidhandlers, so checkout stays broken.Critical: checkout is completely broken on
maintoday —checkout.tscreates a PayMongo Source, but the webhook only handlescheckout_session.*events and dropssource.chargeable/payment.paid, so no purchase can complete.This branch is a hand-assembled reconciliation taking the best of both, plus fixes for 10 bugs found in a review pass over the reconciliation itself. It supersedes #33 and #34.
What's in it
Reconciliation (commits 1–5):
User.claimTokenHashvia colliding migrations) — XpLedger, claim-token columns, uniqueRefundRequest.paymongoRefundId, partial unique indexes for one-active-refund-per-payment (C7) and one-active-cert-per-user-course (H7), email lowercase backfill.Post-review hardening (commit 6) — 10 verified bugs:
handleSourceChargeablenow claims webhook idempotency before charging, runs the PayMongo call outside the DB transaction (no locks held across an HTTP round-trip), releases the claim on charge failure (retries instead of losing the sale), and sends post-purchase emails after commit. H2 now rejects underpayment / non-PHP currency instead of granting access.payment.paidforwards the real payment method instead of defaulting to GCash.ERROR(no orphanedPENDINGrows);/checkout/completeresolves strictly by session id (deadpaymongoReferencefallback removed).issueCertificatecatches the H7 P2002 race and returns the winning row; auto-issue page no longer crashes to the error boundary.requireAdminfails closed when the role lookup returns null.scripts/report-email-collisions.tslists them for manual reconciliation.Test plan
pnpm typecheck— cleanpnpm lint— 0 errors (9 pre-existing warnings, none in touched files)pnpm test— 283 pass (added regression tests: cert P2002 idempotency,requireAdminnull-role fail-closed, source underpayment rejection)pnpm test:coverage— 88.2% lines / 86.4% functions / 77.8% branches / 87.1% statements (>70% gate)pnpm build— succeedsSuperseded
Closes #33 and #34 — do not merge those. If merged, close them as superseded.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SmyprYCco2Hhu48UySiEkk
Generated by Claude Code
Summary by CodeRabbit