Conversation
This reverts commit ef32a47.
Under DRY_RUN=true every run exits through finishDryRun before persistResults (I8), so a planted history drain could never advance — each cron tick re-fetched page 1 and the scan looked stuck forever. analyzeHistory now fails with 409 and leaves the scan state untouched.
|
Skipping CodeAnt AI review — this PR changes more than 100 files, which usually means a migration, codemod, or vendored drop. Line-level review on diffs this large produces duplicate findings on the same rewrite pattern and drowns out anything that actually matters. If you still want a review, comment |
Up to standards ✅🟢 Issues
|
|
@CodeAnt-AI review |
|
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
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 |
|
Running ultrareview automatically — This large PR (329 files) adds a new payment provider (Mercado Pago) with signed webhooks, modifies credit/auto-recharge billing logic, and includes database migration hash repairs; a missed bug could break payments or data integrity.. I'll post findings when complete. |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully integrates Mercado Pago as a payment provider for BRL transactions. After reviewing the security-sensitive areas including webhook endpoints, payment processing, database migrations, and authentication flows, no blocking defects were identified.
The implementation demonstrates solid security practices including proper webhook signature verification with HMAC, timing-safe comparisons, idempotent payment processing, and correct transaction handling. The code follows the existing architectural patterns established for Stripe integration.
Key areas reviewed:
- Mercado Pago webhook signature verification (proper HMAC validation with replay protection)
- Payment fulfillment and reversal logic (idempotent operations)
- Database schema changes (proper indexing and constraints)
- Authentication and authorization checks (requireOrgRole enforcement)
- Secret handling (appropriate use of environment variables)
The PR is ready for merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d5b42d9a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bundle = mercadoPagoBundleById(attempt.bundleId); | ||
| if (bundle.amountCents !== attempt.amountCents) throw new Error('Mercado Pago checkout amount no longer matches the configured catalog'); |
There was a problem hiding this comment.
If a Mercado Pago price variable is changed or removed after a customer opens checkout but before payment approval, mercadoPagoBundleById re-reads the current environment and this path rejects the already-paid transaction, causing every webhook retry to return 500 without granting credits. Persist the purchased credit quantity with the attempt and validate the payment against the persisted amount rather than requiring the live catalog to remain unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — credits are now snapshotted onto the checkout attempt (new nullable mercado_pago_checkout_attempts.credits, migration 0036) at creation time; fulfillment uses the persisted value and only falls back to the live catalog for pre-column rows.
| if (attempt.paymentId && attempt.paymentId !== payment.id) throw new Error('Mercado Pago checkout attempt has a different payment'); | ||
| const bundle = mercadoPagoBundleById(attempt.bundleId); | ||
| if (bundle.amountCents !== attempt.amountCents) throw new Error('Mercado Pago checkout amount no longer matches the configured catalog'); | ||
| if (Math.round(payment.transactionAmount * 100) !== attempt.amountCents) throw new Error('Mercado Pago payment amount does not match the checkout'); |
There was a problem hiding this comment.
When Mercado Pago returns a malformed fractional-cent amount such as 4.999 for a 500-cent attempt, rounding converts it to 500 and grants the credits even though the external amount does not exactly match the checkout. Validate that transactionAmount * 100 is a safe integer and compare it exactly instead of rounding external monetary data.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — replaced Math.round with a Number.isSafeInteger(transactionAmount * 100) guard that throws loudly, then an exact-cents comparison. Fractional-cent amounts are now rejected (I2).
| const counts = $derived([monthOne ?? 0, monthTwo ?? 0, monthThree ?? 0]); | ||
| const validCount = (value: number | undefined) => value === undefined || (Number.isSafeInteger(value) && value >= 0 && value <= MAX_CALCULATOR_COMMENTS); | ||
| const validInputs = $derived(validCount(monthOne) && validCount(monthTwo) && validCount(monthThree)); | ||
| const forecast = $derived(validInputs ? forecastCost(counts) : null); |
There was a problem hiding this comment.
If a visitor fills only one of the three forecast inputs, the other two blank values are silently converted to zero and considered valid, so the page immediately displays an artificially low range. Keep missing values distinct from real zero-comment months and show a forecast only after all three inputs are present and valid.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — the forecast is only computed when all three month inputs are defined and valid (new pure forecastMonths helper in src/lib/landing/cost.ts); blank months no longer count as zero.
| function safeReturnTo(value: FormDataEntryValue | null): string { | ||
| if (typeof value !== 'string' || value === '' || !value.startsWith('/') || value.startsWith('//') || /[\r\n]/.test(value)) { | ||
| throw new Error('locale return path is invalid'); | ||
| } |
There was a problem hiding this comment.
A submitted returnTo such as /\evil.example passes these checks, but browsers resolve that form as a scheme-relative URL, so the POST endpoint can redirect users to an attacker-controlled host. Normalize the value through new URL and require the resulting origin to match the application origin, or reject backslashes outright.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — safeReturnTo now rejects backslashes; /\evil.example returns 400. Regression test added.
| return { | ||
| maintenance: false, | ||
| user, | ||
| mercadoPagoBundles: configuredMercadoPagoBundles(), |
There was a problem hiding this comment.
If any optional Mercado Pago price is present but malformed, configuredMercadoPagoBundles() throws here and the surrounding catch reports the entire Usage page as a database outage, hiding working Stripe controls and displaying an inaccurate maintenance message while the database is healthy. Validate optional provider configuration separately and expose a provider-specific error instead of routing it through the database-maintenance fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — configuredMercadoPagoBundles now loud-logs and skips unmapped/malformed bundles instead of throwing inside the page's try, so a config slip no longer masquerades as a DB outage.
| .from(creditTransactions) | ||
| .where(and(eq(creditTransactions.orgId, orgId), eq(creditTransactions.refType, 'checkout_session'), eq(creditTransactions.refId, providerLedgerRef('mercadopago', payment.id)), eq(creditTransactions.reason, 'purchase'), gt(creditTransactions.delta, 0))) | ||
| .get(); | ||
| if (!grant) throw new Error('Mercado Pago refund arrived before its credit grant; retry is required'); |
There was a problem hiding this comment.
If the approval notification is delayed or missed and the first payment state retrieved is already refunded, there is no purchase ledger row, so this throw asks for a retry that can never succeed: every retry retrieves the same terminal payment state, returns 500, and leaves the attempt pending forever. Mark a terminal payment with no grant as refunded without applying a delta, or persist a reversal obligation that can converge safely if the grant later arrives.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — refund/dispute for a terminal payment with no grant now marks the attempt refunded/disputed, applies no delta, and returns false (converges, no infinite retry).
| export const load: LayoutServerLoad = ({ cookies, request }) => ({ | ||
| locale: resolveLocale({ | ||
| cookie: cookies.get(LOCALE_COOKIE), | ||
| acceptLanguage: request.headers.get('accept-language') | ||
| }) |
There was a problem hiding this comment.
For /terms, /privacy, and /dpa, which each export prerender = true, this ancestor load is evaluated during the build rather than for each visitor, so its cookie and Accept-Language lookup is baked as the build-time locale. After a user selects Portuguese and is redirected back to one of those static pages, the language switcher and layout data still render as English; make locale-dependent pages dynamic or provide locale-specific static variants.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred — valid but architectural: terms/privacy/dpa export prerender = true, so the root layout's locale is baked at build time. Tracked as a deferred entry in TODO.md rather than rushed into this release.
| if (attempt.paymentId) { | ||
| try { | ||
| const applied = await processMercadoPagoPayment(await retrievePayment(attempt.paymentId)); | ||
| return { maintenance: false, user, sessionId: mercadoPagoAttemptId, granted: applied || attempt.status === 'fulfilled', pending: !applied, failed: false }; |
There was a problem hiding this comment.
When an attempt has already been marked refunded or disputed, it has a payment ID and reaches this branch; replaying the idempotent reversal returns false, which is then translated into pending: true. Refreshing the success URL therefore shows “Payment received — almost there” indefinitely for a payment that has already been reversed, so handle the refunded and disputed statuses explicitly before treating a false application result as pending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — refunded/disputed attempts now render an explicit failed state before the paymentId branch; regression test asserts retrievePayment is never called for them.
| if (attempt.status === 'fulfilled') throw new Error('Mercado Pago checkout attempt has already completed'); | ||
| if (attempt.initPoint) return attempt.initPoint; |
There was a problem hiding this comment.
After an attempt is marked refunded or disputed, resubmitting its retry-safe attempt ID passes the fulfilled-only guard and returns the original preference URL. That URL is either already terminal or can produce a second payment whose new payment ID fulfillment rejects as belonging to a different payment, leaving the customer without the newly purchased credits; treat every reversed state as terminal and require a fresh attempt.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — refunded/disputed attempts are treated as terminal and refuse reuse with a loud descriptive error, for both statuses.
| if (typeof body.id !== 'number' && typeof body.id !== 'string') throw new Error('Mercado Pago payment has no id'); | ||
| if (typeof body.status !== 'string' || typeof body.external_reference !== 'string') throw new Error('Mercado Pago payment has invalid status or reference'); | ||
| if (typeof body.transaction_amount !== 'number' || !Number.isFinite(body.transaction_amount) || body.transaction_amount <= 0) throw new Error('Mercado Pago payment has invalid amount'); | ||
| if (typeof body.currency_id !== 'string') throw new Error('Mercado Pago payment has invalid currency'); | ||
| const refundedAmount = body.transaction_amount_refunded; | ||
| if (refundedAmount !== undefined && (typeof refundedAmount !== 'number' || !Number.isFinite(refundedAmount) || refundedAmount < 0)) throw new Error('Mercado Pago payment has invalid refunded amount'); | ||
| return { | ||
| id: String(body.id), |
There was a problem hiding this comment.
The payment lookup validates that the response contains an ID but never verifies it matches paymentId. If Mercado Pago or an intermediary returns a stale or mismatched payment object, the signed notification for one payment can therefore fulfill the different payment referenced by the response body, including its organization and attempt; compare the normalized response ID with the requested ID before returning it.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — retrievePayment now throws when String(body.id) !== paymentId.
- hooks.server.ts: drop unnecessary \" escapes in single-quoted literals (S6535) - mercadopago/webhooks.ts: optional-chain the attempt guards (S6582) - usage/success: extract per-provider helpers — cognitive complexity 23 → under 15 (S3776) Behavior-preserving; the existing suites (hooks.server, mercadopago webhooks, usage/success) pin every touched branch and stay green.
|
Triage (commit f37cbeb) @sonarqubecloud — Quality Gate passed, 7 new issues. All triaged as valid and fixed:
All three are behavior-preserving, so no new failing behavior test applies — the existing suites ( @codacy-production — 0 issues. Nothing to do. @coderabbitai — review skipped (329 files > 100-file limit): this PR is the batched @CodeAnt-AI — initially skipped for size; review was requested and is in progress. Will triage any findings when they land. @Amazon-Q-Developer — review completed with no blocking defects on the Mercado Pago integration. Acknowledged. |
|
Tip For best results, initiate chat on the files or code changes.
The SonarQube findings are fixed and verified by the reported checks. The release PR exceeds the line-level review limit. The per-PR reviews on the changes merged into ✏️ Learnings added
You are interacting with an AI system. |
|
Question: -
all three are behavior-preserving, so no new failing behavior test applies — the existing suites ( @codacy-production — 0 issues. nothing to do. @coderabbitai — review skipped (329 files > 100-file limit): this pr is the batched @CodeAnt-AI — initially skipped for size; review was requested and is in progress. will triage any findings when they land. @Amazon-Q-Developer — review completed with no blocking defects on the mercado pago integration. acknowledged. Answer: Findings
The other three changes described in the PR questions appear behavior-preserving and do not raise additional issues from the supplied diff. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 24m 17s
10 issues found and verified against the latest diff
Confidence score: 1/5
drizzle/0010_users_deleted_at_idx.sqlchanges an already-applied migration and will trigger the deployment hash check, blocking deployments; restore the applied file unchanged and add a new migration for any schema change.DEPLOY.mdexposes shared development database credentials and service secrets to untrusted PR preview code, creating a serious security boundary failure; use isolated, non-sensitive preview credentials before enabling this workflow.src/lib/landing/cost.tsunderstates hosted pricing for zero-comment accounts and for overage purchased in bundles, so customers can see materially incorrect monthly costs; include the hosted base plan and configured bundle pricing.src/lib/components/LanguageSwitcher.sveltedrops thestatequery parameter in no-JavaScript redirects, causing consent and channel-connection flows to restart or fail; preserve the full query string in the SSR fallback and cover these paths.
Not reviewed (too large): reports/mutation/mutation.html (~2 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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="drizzle/0035_mercado_pago_credit_checkout.sql">
<violation number="1" location="drizzle/0035_mercado_pago_credit_checkout.sql:24">
P2: The `payment_id` column already has a unique index (`mercado_pago_checkout_attempts_payment_id_unique`). This additional non-unique index on the exact same column is redundant and wastes storage while slowing down inserts and updates. Remove this index from both this migration and the `index()` array in `schema.ts`.</violation>
</file>
<file name="drizzle/0010_users_deleted_at_idx.sql">
<violation number="1" location="drizzle/0010_users_deleted_at_idx.sql:14">
P0: Modifying an already applied migration file changes its SHA-256 hash. The deployment build gate runs `scripts/verify-migrations.mjs`, which will detect a hash mismatch against the `__drizzle_migrations` table and block the deployment. Revert this comment update to preserve the original file hash.</violation>
</file>
<file name="src/routes/api/mercadopago/webhook/+server.ts">
<violation number="1" location="src/routes/api/mercadopago/webhook/+server.ts:21">
P3: This new public webhook has no route-level tests, so malformed payload, signature, and 400/500 response behavior can regress unnoticed. Add a `webhook.test.ts` that invokes `POST` with mocked helpers and covers those boundary cases.</violation>
</file>
<file name="DEPLOY.md">
<violation number="1" location="DEPLOY.md:33">
P1: The recommended `*/15 * * * *` schedule runs every 15 minutes, not more frequently than `* * * * *`; with N channels it changes cadence from N to 15N minutes and delays moderation. Keep the one-minute schedule or document the slower cadence as an explicit tradeoff.</violation>
<violation number="2" location="DEPLOY.md:123">
P0: When the documented `branch-deploys` context serves PR previews, untrusted PR server code receives the shared dev database token and other service secrets. Skipping `netlify-migrate` only skips migrations; give previews no secrets or an isolated database.</violation>
</file>
<file name="src/lib/landing/cost.ts">
<violation number="1" location="src/lib/landing/cost.ts:18">
P2: When overage is bought through the supported credit flow, it is charged as a configured bundle rather than per comment, so this calculator understates spend for non-bundle-sized overage and diverges when bundle prices change. Derive the estimate from the bundle sizes and configured prices, or label it as an effective usage rate instead of monthly spend.</violation>
<violation number="2" location="src/lib/landing/cost.ts:30">
P1: When a hosted customer has zero comments, this function reports `$0/month` even though the hosted subscription still renews at `$5/month`; the free tier is self-hosted only. Return `MONTHLY_PLAN_USD` for zero comments, or model the self-hosted choice separately from `hostedCostUsd`.</violation>
</file>
<file name="src/lib/components/LanguageSwitcher.svelte">
<violation number="1" location="src/lib/components/LanguageSwitcher.svelte:10">
P2: When JavaScript is disabled, this SSR value contains only the pathname. On `/consent?state=...` or `/connect-channel?state=...`, the redirect drops `state`, restarting consent or producing a missing-state error. It also always drops fragments such as `/terms#s1`; include `page.url.search` and the browser-only `page.url.hash`.</violation>
</file>
<file name="TODO.md">
<violation number="1" location="TODO.md:28">
P1: When Mercado Pago reports a `charged_back` payment without a refund amount, this completion claim is false: processing rejects the dispute before reversing the credit ledger. Enforce the full-refund amount only for refund events and add a charged-back reversal test before checking this item off.</violation>
<violation number="2" location="TODO.md:83">
P2: The dev verification covers 36 journal entries, not 35, so this checked release claim understates the schema scope. Change the checklist count to 36.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| -- repository root. | ||
| -- | ||
| -- Commercial licensing: contact@marketingprowess.simplelogin.com — see COMMERCIAL.md | ||
| -- Commercial licensing: contact@AdvancedDigitalMarketingLTDA.com — see COMMERCIAL.md |
There was a problem hiding this comment.
P0: Modifying an already applied migration file changes its SHA-256 hash. The deployment build gate runs scripts/verify-migrations.mjs, which will detect a hash mismatch against the __drizzle_migrations table and block the deployment. Revert this comment update to preserve the original file hash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At drizzle/0010_users_deleted_at_idx.sql, line 14:
<comment>Modifying an already applied migration file changes its SHA-256 hash. The deployment build gate runs `scripts/verify-migrations.mjs`, which will detect a hash mismatch against the `__drizzle_migrations` table and block the deployment. Revert this comment update to preserve the original file hash.</comment>
<file context>
@@ -11,6 +11,6 @@
-- repository root.
--
--- Commercial licensing: contact@marketingprowess.simplelogin.com — see COMMERCIAL.md
+-- Commercial licensing: contact@AdvancedDigitalMarketingLTDA.com — see COMMERCIAL.md
CREATE INDEX `users_deleted_at_idx` ON `users` (`deleted_at`);
</file context>
There was a problem hiding this comment.
Valid concern, wrong remedy — reverting would drift the dev database instead (it already matches the current hashes). The real repair is bookkeeping reconciliation, and it is now fully documented: DEPLOY.md §1 carries the exact prod-verified old→new hash pairs (positional mapping checked against production 2026-08-25) plus the scripts/reconcile-migrations.mjs attested workflow, and AGENTS.md gained the "never edit an applied migration file, not even comments" rule.
| context (the `dev` branch and PR previews) gets the dev Google OAuth | ||
| client and the dev Turso database. This is what keeps dev from touching |
There was a problem hiding this comment.
P0: When the documented branch-deploys context serves PR previews, untrusted PR server code receives the shared dev database token and other service secrets. Skipping netlify-migrate only skips migrations; give previews no secrets or an isolated database.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At DEPLOY.md, line 123:
<comment>When the documented `branch-deploys` context serves PR previews, untrusted PR server code receives the shared dev database token and other service secrets. Skipping `netlify-migrate` only skips migrations; give previews no secrets or an isolated database.</comment>
<file context>
@@ -0,0 +1,242 @@
+- **Two environments, two deploy contexts.** Set the same keys twice:
+ the **production** context (`main` deploys) gets the production Google
+ OAuth client and the production Turso database; the **branch-deploys**
+ context (the `dev` branch and PR previews) gets the dev Google OAuth
+ client and the dev Turso database. This is what keeps dev from touching
+ production — do not point either context at the other's resources.
</file context>
| context (the `dev` branch and PR previews) gets the dev Google OAuth | |
| client and the dev Turso database. This is what keeps dev from touching | |
| context (the `dev` branch only) gets the dev Google OAuth client and the | |
| dev Turso database; Deploy Previews get no database or service secrets, or | |
| use an isolated ephemeral database. This is what keeps dev from touching |
There was a problem hiding this comment.
Maintainer's call — this is Netlify deploy-context configuration (which env vars the branch-deploys context exposes to previews), not repo code. Flagged to the human; no code change.
| @@ -0,0 +1,46 @@ | |||
| // Moderaty — YouTube Comment Auto-Moderation Tool | |||
There was a problem hiding this comment.
P3: This new public webhook has no route-level tests, so malformed payload, signature, and 400/500 response behavior can regress unnoticed. Add a webhook.test.ts that invokes POST with mocked helpers and covers those boundary cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/api/mercadopago/webhook/+server.ts, line 21:
<comment>This new public webhook has no route-level tests, so malformed payload, signature, and 400/500 response behavior can regress unnoticed. Add a `webhook.test.ts` that invokes `POST` with mocked helpers and covers those boundary cases.</comment>
<file context>
@@ -0,0 +1,46 @@
+import { retrievePayment } from '$lib/server/mercadopago/client';
+import { processMercadoPagoPayment, verifyWebhookSignature } from '$lib/server/mercadopago/webhooks';
+
+export async function POST({ request }) {
+ let payload: unknown;
+ try {
</file context>
There was a problem hiding this comment.
Fixed in 925f75f — webhook.test.ts now covers malformed payloads, signature failure (400), internal errors (500), and log-safe payment IDs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f37cbeb81a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async function reverseMercadoPagoPayment(payment: MercadoPagoPayment, reason: 'refund' | 'dispute'): Promise<boolean> { | ||
| if (payment.refundedAmount !== payment.transactionAmount) { | ||
| throw new Error('Mercado Pago payment is not a full refund'); |
There was a problem hiding this comment.
When Mercado Pago reports status === 'charged_back' while transaction_amount_refunded remains 0 (a chargeback rather than a provider refund), this shared guard throws before disabling auto-top-up or recording the dispute reversal. Every retry retrieves the same terminal payment and fails again, leaving the disputed credits usable indefinitely; apply the full-refund check only to refunds and reverse chargebacks from the original grant.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 925f75f — disputes no longer require refundedAmount === transactionAmount; a chargeback with no refund amount reverses the full amount. charged_back regression test included.
| const sessionId = url.searchParams.get('session_id'); | ||
| const mercadoPagoAttemptId = url.searchParams.get('attempt_id'); | ||
| if (url.searchParams.get('provider') === 'mercadopago' && mercadoPagoAttemptId) { | ||
| return mercadoPagoSuccess(user, mercadoPagoAttemptId); |
There was a problem hiding this comment.
When the customer redirect arrives before the webhook, the attempt's paymentId is still null because only webhook processing writes it, so mercadoPagoSuccess cannot retrieve the approved payment and always shows pending. The Mercado Pago return includes the payment identifier, but this load reads only attempt_id; validate and process that returned payment ID against the owned attempt so this fallback actually fulfills delayed or missed notifications.
Useful? React with 👍 / 👎.
| const initPoint = environment() === 'sandbox' ? body.sandbox_init_point : body.init_point; | ||
| if (typeof body.id !== 'string' || body.id.length === 0) throw new Error('Mercado Pago preference has no id'); | ||
| if (typeof initPoint !== 'string' || initPoint.length === 0) throw new Error('Mercado Pago preference has no checkout URL'); | ||
| return { id: body.id, initPoint }; |
There was a problem hiding this comment.
If the preference API returns an unexpected init_point or sandbox_init_point, this accepts any nonempty string, persists it, and the Usage action sends it directly to redirect(303, url), turning a malformed or compromised upstream response into a trusted open redirect. Parse the value with new URL, require HTTPS and an approved Mercado Pago checkout origin, and return only the validated URL.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
| .reasonix | ||
| .aimax | ||
| .codacy | ||
| .agents No newline at end of file |
There was a problem hiding this comment.
Ignoring the entire .agents tree also ignores the repository-mandated .agents/skills-src/<name>/ sources, so normal git add operations will silently omit new skills and updates to the canonical sources used by the install workflow. Ignore only generated/local AI MAX artifacts, or explicitly re-include .agents/skills-src/.
AGENTS.md reference: AGENTS.md:L44-L53
Useful? React with 👍 / 👎.
| // before persistResults (I8), so the planted drain can never advance — | ||
| // each cron tick re-fetches page 1 and the scan looks stuck forever. | ||
| // Refuse loudly instead of planting a scan that cannot moderate. | ||
| if (env.DRY_RUN === 'true') { |
There was a problem hiding this comment.
When DRY_RUN is missing or misspelled, this condition treats the deployment as live and persists the history cursor reset, but runChannel later rejects every invocation because it accepts only true or false; the planted scan is therefore left unable to advance. Validate the required variable before any database mutation and proceed only when its value is exactly false.
AGENTS.md reference: AGENTS.md:L143-L145
Useful? React with 👍 / 👎.
| import { t } from '$lib/i18n/messages'; | ||
|
|
||
| let { locale }: { locale: Locale } = $props(); | ||
| const returnTo = $derived(`${page.url.pathname}${browser ? page.url.search : ''}`); |
There was a problem hiding this comment.
On query-dependent pages rendered without JavaScript, this omits the query string from the language form's hidden returnTo; for example, switching language on /consent?state=... redirects to /consent without the parked OAuth state, so account creation is abandoned and the user is sent back to login. page.url.search is available during SSR, so include it unconditionally rather than gating it on browser.
Useful? React with 👍 / 👎.
| } catch (cause) { | ||
| console.error(`Mercado Pago webhook failed for payment ${paymentId}:`, cause); | ||
| return json({ error: 'Mercado Pago webhook processing failed' }, { status: 500 }); |
There was a problem hiding this comment.
An unauthenticated caller can submit a string payment ID containing newlines or a very large value; signature verification then throws before retrievePayment reaches its ID validator, and this catch interpolates the raw value into server logs. Validate and bound the body ID before the try, and log only the normalized value or a short hash, to prevent forged log entries and attacker-controlled log amplification.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Triage round 2 & 3 (commits @CodeAnt-AI — your [High] double-reversal finding (refund + chargeback subtracting twice via reason-keyed idempotency) was valid and is fixed in Codex rounds — all 17 threads dispositioned, 15 fixed:
@cubic-dev-ai — all 24 threads dispositioned. Fixed: Ticker, locale open redirect, bundles, charged_back full-amount trap, 0035 redundant index (dropped via new migration 0036 — 0035 untouched, it's applied), COOLIFY_BUNNY/ Migration drift findings (cubic P0 on 0010, codex P1 on 0000) — not reverting the migration files: dev-2 already matches the current hashes, so a revert just moves the drift. Instead: DEPLOY.md §1 now has the exact repair runbook (verified against production 2026-08-25 — 20 applied rows, 16 drifted, positional mapping confirmed, 14 header-only + 2 schema-equivalent drifts documented individually), the attested Flagged for the maintainer (your call, no code change):
@sonarqubecloud — 0 issues since |
|
Tip For best results, initiate chat on the files or code changes.
The reported fixes and regression coverage address the valid findings from the external reviews. The migration-drift runbook and the rule against editing applied migrations are appropriate safeguards. The three flagged items require maintainer decisions. The deferred legal-page localization work is tracked separately in For this large 🧠 Learnings usedYou are interacting with an AI system. |
@Bonobo791 All 24 cubic threads check out against what's in this PR — no outstanding findings. The dispositions map cleanly to the fixes I flagged, and the two deferred/architectural items (prerendered legal-page locale baking) staying in TODO.md is fine while there's no pt-BR legal bundle live. The migration-drift resolution is the right call given your constraint: dev-2 already matches the current hashes, so reverting the files would just relocate the drift rather than clear it. Documenting the exact verified old→new hashes plus the attested On the three maintainer-flagged items:
2/3 are presentation and Netlify-config matters, not code — agreed on keeping them out of the PR. No further action needed from me here. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
|
Question: reversemercadopagopayment` now checks the ledger for any existing reversal (refund or dispute) on the same payment before applying, and the regression test processes chargeback→refund and asserts exactly one negative ledger entry. the attempt status still converges to the latest terminal state. codex rounds — all 17 threads dispositioned, 15 fixed:
@cubic-dev-ai — all 24 threads dispositioned. fixed: ticker, locale open redirect, bundles, charged_back full-amount trap, 0035 redundant index (dropped via new migration 0036 — 0035 untouched, it's applied), coolify_bunny/ migration drift findings (cubic p0 on 0010, codex p1 on 0000) — not reverting the migration files: dev-2 already matches the current hashes, so a revert just moves the drift. instead: deploy.md §1 now has the exact repair runbook (verified against production 2026-08-25 — 20 applied rows, 16 drifted, positional mapping confirmed, 14 header-only + 2 schema-equivalent drifts documented individually), the attested flagged for the maintainer (your call, no code change):
@sonarqubecloud — 0 issues since Answer:
For example:
The success page avoids this path because it checks terminal status, but the webhook endpoint does not. Add the persisted status to the fulfillment query and reject/no-op when it is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fe98cfbc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // (approved/refunded/charged_back) or a valid no-op (pending/in_process/ | ||
| // rejected/cancelled). Anything else is out of contract — the call failed (I2), | ||
| // never a silent fall-through that answers the webhook 200 without fulfilling. | ||
| const KNOWN_PAYMENT_STATUSES: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
Fresh evidence after the earlier status-enum review is that this new allowlist still omits Mercado Pago's valid authorized and in_mediation states. When a payment is awaiting capture or is under mediation, retrievePayment therefore throws before processMercadoPagoPayment can treat the state as a valid no-op, causing every notification for that potentially long-lived state to return 500 and generate unnecessary retries and failure logs; include the complete provider status enum while continuing to reject genuinely unknown values.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3602123 — allowlist now covers the full valid enum including authorized/in_mediation; long-lived legitimate states no longer storm 500-retries. Tests updated.
| attemptId, | ||
| orgId, | ||
| bundleId: bundle.id, | ||
| idempotencyKey: `mercadopago:checkout:${attemptId}:${randomUUID()}`, |
There was a problem hiding this comment.
Mercado Pago limits X-Idempotency-Key values to 64 characters, but even the normal UUID path constructs a 94-character value here (mercadopago:checkout: plus two UUIDs), and a caller-supplied attempt ID can make it longer still. The preference request therefore fails provider validation before checkout can open; persist a provider-valid UUID or fixed-length hash as the retry key instead.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3602123 — the idempotency key is now sha256("mercadopago:checkout:<attemptId>") hex: exactly 64 chars, deterministic per attempt. Tests pin length/stability/uniqueness.
| if (reversal) { | ||
| await markTerminal(); | ||
| return false; |
There was a problem hiding this comment.
When a full refund has already created the payment-level reversal and Mercado Pago later reports the same payment as charged_back, this early return marks the attempt disputed but skips the reason === 'dispute' branch that disables automatic top-up. Credits are correctly not subtracted twice, but the organization remains eligible for further off-session charges after a chargeback; apply the dispute side effect before returning from the deduplication path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3602123 — the dispute side effect (disable auto top-up) is applied in the dedup early-return path too; refund-then-chargeback disables auto top-up with exactly one reversal row. Tested.
…r impede migration hashing
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea9e5d2308
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The returned init_point is persisted and later used in redirect(303, url): | ||
| // only https Mercado Pago checkout origins are trusted — www.mercadopago.com, | ||
| // its subdomains, and the country hosts (www.mercadopago.com.br, .com.ar, …). | ||
| const CHECKOUT_HOST = /(^|\.)mercadopago\.com(\.[a-z]{2})?$/; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier unrestricted-URL finding is that the replacement regex still accepts every two-letter suffix, including separately registrable origins such as mercadopago.com.io and all their subdomains. If a malformed or compromised preference response supplies a matching HTTPS URL, checkoutUrl approves it and the Usage action redirects the customer there; use an explicit allowlist of Mercado Pago's documented checkout hosts instead of the country-code wildcard.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3602123 — the country-code wildcard is replaced by an explicit host allowlist (apex mercadopago.com + subdomains, www|sandbox.mercadopago.com.{br,ar,mx,cl,co,pe,uy}); mercadopago.com.io and lookalikes are rejected. Tests updated.
| .get(); | ||
| if (!existing) throw new Error(`Mercado Pago checkout attempt ${attemptId} disappeared while creating`); | ||
| if (existing.orgId !== orgId || existing.bundleId !== bundle.id) throw new Error('checkout attempt does not belong to this purchase'); | ||
| if (existing.amountCents !== bundle.amountCents) throw new Error('checkout attempt price changed; start a new checkout'); |
There was a problem hiding this comment.
Fresh evidence after the persisted-credits fix is that loadOrCreateAttempt still reloads only the amount and validates only that field. If a request plants an attempt and fails before creating the preference, then a deploy changes the bundle's credit count without changing its BRL price, retrying the same attempt advertises the new bundle.credits to Mercado Pago while fulfillment grants the older persisted credits; reload and use the snapshot, or reject a credit-count mismatch before resuming.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3602123 — resuming a pending attempt now reloads the persisted credits snapshot and uses it for the preference, so preference and fulfillment always agree. Test asserts the snapshot wins over the live catalog.
|
Triage round 3 (commit Themes this round (mostly races/edge cases exposed by the round-1/2 fixes — the re-review loop working as intended):
Nothing skipped; nothing flagged for maintainer decision this round. One caveat noted by the implementer: the true-concurrency reversal test can't distinguish pre/post-fix on the single-connection in-memory harness (the pre-fix race surfaces there as a loud |
|
@CodeAnt-AI — your follow-up [P1] fulfill-after-terminal-reversal was valid against the round-1 code and is fixed in |
@Bonobo791 The Mercado Pago closed-loop behavior aligns with what's implemented: webhook notifications are retried until a 2xx, and statuses include You're covered on both fronts given the round-3 fixes:
And since Mercado Pago marks a successful ack with a 200/201 but still may deliver overlapping update notifications for the same payment, both dedup paths (fulfillment claim + reversal transaction) being idempotent is the right posture. The one thing I'd keep an eye on for the retry semantics: Mercado Pago retries for 22 seconds before giving up and re-queueing every 15 minutes. Netlify's default function timeout being 10s (below your 26s recommendation in DEPLOY.md §5) matters more than ever here — a webhook hit during a busy moment that times out past 22s triggers a 15-minute replay loop, so the elevated timeout is doing real work for the webhook path too, not just cron. Worth confirming the webhook function also inherits that raised limit. No further action from me. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
|
Question: fulfillmercadopagopayment Answer:
One wording detail: the actual predicate is slightly broader than or(
isNull(mercadoPagoCheckoutAttempts.paymentId),
eq(mercadoPagoCheckoutAttempts.paymentId, payment.id)
)That is intentional for the initial fulfillment claim, since the payment ID is normally unset. The terminal-state exclusion and subsequent re-read provide the required race protection. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36021230eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!grant) { | ||
| await markTerminal(); | ||
| return false; |
There was a problem hiding this comment.
When a charged_back payment arrives before its purchase grant, this !grant branch marks the attempt disputed and returns without calling disableAutoTopup(), leaving the organization eligible for subsequent off-session Stripe charges despite the dispute. Fresh evidence beyond the earlier deduplication finding is that the newly added no-grant terminal path still skips the dispute side effect; invoke it before returning, as the other chargeback paths do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 24277e3 — the no-grant terminal branch now calls disableAutoTopup() before marking the attempt terminal (mirroring the dedup path; it no-ops for refunds). Tests: chargeback-before-grant disables auto top-up with zero ledger rows; a no-grant refund leaves top-up untouched.
| } | ||
|
|
||
| export async function processMercadoPagoPayment(payment: MercadoPagoPayment): Promise<boolean> { | ||
| if (payment.status === 'approved') return fulfillMercadoPagoPayment(payment); |
There was a problem hiding this comment.
When Mercado Pago represents a partial refund as an approved payment with 0 < refundedAmount < transactionAmount, this dispatches directly to fulfillment; an existing grant becomes an idempotent no-op and the webhook returns 200 without any error or manual-review signal. That contradicts the deployment check in DEPLOY.md that partial refunds are rejected for manual review, so inspect refundedAmount before accepting the approved state and fail loudly for a partial refund.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 24277e3 — fulfillment now throws loudly on any non-zero refundedAmount: partial refund → "rejected for manual review" (matching DEPLOY.md §3), full-covering amount on an approved payment → out-of-contract throw (I2). Nothing is granted and the webhook 500s so MP keeps retrying for an operator. Tests cover both shapes.
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
@cubic-dev-ai — good catch on the timeout interaction. Confirmed: the 26s limit is a site-level Netlify setting (Site settings → Functions), and adapter-netlify serves every SSR route — including |
|
Triage round 4 (commit
@CodeAnt-AI — acknowledged your self-resolved follow-up; the wording nuance you noted ( |
|
@Bonobo791 Confirmed, and the doc covers it exactly. I read back §5 — it now spells out that adapter-netlify serves every SSR route (including Nothing further from me on this thread. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
@Bonobo791 Round 4 dispositions check out against the code I reviewed — no outstanding findings.
The one caveat I'd keep in mind from earlier still stands: after the production migration repair, confirm the row count is exactly 37 and Tip: get faster answers by chatting with cubic's review copilot in the review UI Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
|
Question: check` 0/0, 1721/1721 tests, build green, codacy clean.
@CodeAnt-AI — acknowledged your self-resolved follow-up; the wording nuance you noted ( Answer:
The timeout clarification is also documented in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a590279ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // exclusion binds the SUPPORTED LOCALE it names: a broader range ('pt;q=1') | ||
| // must not smuggle back a locale an exact range excluded ('pt-BR;q=0') | ||
| // (cubic/codex, PR #136 round 3). | ||
| const excluded = new Set(preferences.filter((preference) => preference.q <= 0).map((preference) => supportedLocaleFor(preference.language))); |
There was a problem hiding this comment.
For an Accept-Language header such as pt;q=0,pt-BR;q=1, this pre-computation maps both ranges to pt-BR and globally marks that locale excluded, so the explicit exact preference can never win and the resolver returns English. Evaluate each supported locale using its most-specific matching range so a broad exclusion does not override a more-specific positive preference.
Useful? React with 👍 / 👎.
|
|
||
| export async function retrievePayment(paymentId: string): Promise<MercadoPagoPayment> { | ||
| if (!/^[A-Za-z0-9_-]{1,128}$/.test(paymentId)) throw new Error('Mercado Pago payment id is invalid'); | ||
| const response = await fetchWithRetry(apiUrl(`/v1/payments/${encodeURIComponent(paymentId)}`), { |
There was a problem hiding this comment.
When Mercado Pago stalls or returns a long Retry-After, this call uses fetchWithRetry without a deadline. That helper can spend four 10-second attempts plus backoffs—and honors an unbounded provider retry delay—while the documented Netlify function timeout is only 26 seconds, so the platform can terminate the webhook before this handler returns its retriable 500, delaying credit grants or refund reversals. Pass an absolute deadline that leaves time for payment processing and the response.
AGENTS.md reference: AGENTS.md:L157-L158
Useful? React with 👍 / 👎.
| * the same idempotent processor the webhook uses; otherwise still pending. | ||
| */ | ||
| async function mercadoPagoSuccess(user: SessionUser, attemptId: string): Promise<SuccessState> { | ||
| const attempt = await db |
There was a problem hiding this comment.
If the database becomes unavailable after the request hook completes but before this attempt lookup, the query rejects outside every try block and the entire /usage/success load throws. The customer then receives the raw application error instead of the page's existing maintenance/error state and cannot determine whether the just-completed payment was recorded; catch and log this lookup failure and return a user-visible maintenance or error state.
AGENTS.md reference: AGENTS.md:L122-L124
Useful? React with 👍 / 👎.
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
Bonobo791
changed the title
1 participant
What ships
Mercado Pago prepaid credit checkout (BRL) — second billing provider alongside Stripe:
(new
mercado_pago_checkout_attempts.credits, migration 0036), and a 64-charsha256 idempotency key (provider limit) that includes the org id.
/api/mercadopago/webhook) with typed signature errors(400) vs config errors (500 retriable), checkout-URL host allowlist, and payment
status/amount/currency contract validation.
not per-reason), conditional fulfillment claim that cannot grant after a terminal
reversal, auto top-up disabled on every chargeback path, partial refunds rejected
loudly for manual review per DEPLOY.md §3.
i18n foundation + Portuguese (pt-BR) — Accept-Language/cookie locale resolution
(RFC 7231 q-value semantics incl. q=0 exclusions), language switcher, localized
landing and consent flows,
/api/localeendpoint,docs/I18N.md(step 54).Landing cost calculators — hosted vs self-hosted monthly cost estimator with
validated three-month forecasting (step 55).
Deployment runbook restored and reconciled — DEPLOY.md §1 now carries the exact
production migration-hash repair (15 drifted rows, positional mapping verified
against prod 2026-08-25) using the attested
scripts/reconcile-migrations.mjs;raised 26s function timeout documented as covering the MP webhook path too.
Migration hygiene — license headers stripped from all migration SQL files
(generated artifacts; the deploy gate hashes contents). AGENTS.md rule: never edit
an applied migration and never add headers/comments to migration files. Dev-2
reconciled and verified (37/37 PASS).
Dry-run safety — analyze-history refuses loudly on
DRY_RUN=truedeploymentsinstead of looking stuck.
Review hardening — 4 triage rounds, 40+ verified bot findings fixed
test-first (SonarCloud, CodeAnt, Codex, cubic): billing races, open redirects,
locale edge cases, test hygiene. All dispositions posted on the PR.
Verification
npm run check0 errors/0 warnings;npm run test1721/1721;npm run buildgreenPRAGMA table_info+ applied-count)Release notes for the operator
with 15 MISSING + 15 EXTRA (expected — hash drift from the old license headers).
Follow DEPLOY.md §1: dump prod hashes →
RECONCILE_EXPECTED_HASHES=… node scripts/reconcile-migrations.mjs→
npm run db:verify→ redeploy. Human-only step (production DB).backfill rows (
UPDATE channels SET history_next_page_token = NULL WHERE …).STRIPE_PRICE_HOSTED_MONTHLY,STRIPE_PRICE_LIFETIME,Mercado Pago keys per context (dev =
sandbox).contact change, pricing-calculator framing, Netlify preview secrets scope.