amadan
Multi-currency: the fifteenth review's findings
Nothing records on an intent under an open refusal review: the record's transaction consumes the intent only when no review of it is open, and a purchase corrected at the provider waits for the operator, whose resolution now retires the intent (the default) or keeps it standing for the corrected purchase to record. A retired intent's subscription events are settled before any validation whatever intent stands. The provisioning script says the book's 36 keys; comments, the design and the dispositions say what the code does. The fifteenth review's reports; dispositions "Review fifteen".
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
9 files changed,
+416
−20
docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md+1 −1docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review15.md+114 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+16 −1docs/superpowers/specs/OPUS-REVIEW-multi-currency-review15.md+105 −0implementation/carloku-paid-launch/stripe-provision.sh+1 −1internal/console/billingcurrency_test.go+132 −0internal/console/billingoperator.go+21 −2internal/console/billingservice.go+12 −13internal/console/billingstore.go+14 −2
CI passed — run details
diff --git a/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md b/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md| index f456f052a..2c333189e 100644 |
| --- a/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md |
| +++ b/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md |
| @@ -149,7 +149,7 @@ New columns, both `TEXT NOT NULL DEFAULT ''`, on `billing_checkout_intents` and |
| - **Checkout** (`POST /a/{sqid}/billing/checkout`): one form, with the country and currency controls, the plan, the postal code and the consent. The form carries what the page priced (`quoted_country`, `quoted_currency`) and whether the currency shown was a choice (`currency_chosen`). A post whose country or currency is not what the page priced, judged on what would be bought (the chosen currency, else the posted country's own), or whose quote the page did not sign (`quote_sig`: an HMAC over the session token, the account, the book, the quoted country and its band at render time, the quoted currency, the choice flag and the member count the page priced at, keyed by a secret derived from the billing secret key, which only the server holds), is not a purchase: it is answered with a redirect to the page quoting the posted country (and the posted currency when it is a choice), so nothing is bought at a price a page did not show this session. The member count the page priced at is compared again when the door charges (under the billing lock; the intent snapshot's membership-locked check then holds that count against the account); a count that moved is a conflict (`?error=conflict`, the existing banner), not a higher charge. A currency shown as the country's default and not chosen is given to the door as no currency, and the billing country's own currency is bought in; a chosen one (the URL's, the pricing page's, the account's, or changed on the form) is the purchase's. A currency field that is present and is not exactly `usd`, `eur` or `gbp` (no trimming, no case folding, not empty): 400, nothing is created, before the quote is looked at; on the page, a `?currency=` that names such a thing is said (`billing.currency.unknown`, unreviewed system copy like `billing.quote.unknown`) and the default shown is not a choice. The quote comparison is made on what would be bought (the chosen currency, else the posted country's own), so a form whose fields disagree with each other, a choice flag erased, or a form with no currency control against a page that priced a choice, is answered with the quote for what it would buy. The intent stores the chosen currency and book. A retry with a different currency while an intent is open is a different intent, exactly as a different plan is today. `POST /a/{sqid}/billing/quote` is the form's "Show prices for this country" button: the same form, answered with the same redirect, buying nothing; it asks for the session and the form's token, no step-up. |
| - **A currency already held**: checkout is refused while the account's subscription has not ended (`canceled` or `incomplete_expired`), and after it has ended a checkout in any currency but the one bought in is refused, the billing country's default included; the page offers that currency alone, as a choice, for a purchase from before price books too (EUR). A change of currency waits on what Stripe permits for a customer whose subscription ended (test-mode item 4) and is then an operator's door, not the form's. |
| - **Stripe price identity**: the configured price for (book, plan, band, currency) must have that `currency`, `unit_amount` equal to the book's unit price, no `currency_options` but, at most, an entry under its own currency at its own amount and tax behaviour (which an expanded read of a single-currency price may carry), and metadata `price_book`, `currency`, `terms_version`, `plan`, `band` that all match. The checks that exist today (per-unit, yearly, licensed, exclusive tax, active, mode) stay. |
| -- **Checkout Session**: created with `currency` and `adaptive_pricing[enabled]=false`, and with `currency`, `price_book` in the session and subscription metadata. A purchase with no book (only a console with no book configured opens one, and an intent from before books is replayed as that build sent it) is asked for exactly as before: no currency, Adaptive Pricing on. Every session the console creates, replays, lists, reads back or binds passes one check against its intent; one that is this intent's and not its purchase (another currency, a conversion, Adaptive Pricing) is expired and the intent retired, and one already paid is left for a person with a review filed once. A subscription whose price is not the configured one for its stored (book, plan, band, currency), or that the provider reports in another currency, is an identity mismatch. Once an intent is bound, a read of its bound session that names another account, customer, intent or mode is a contradiction at the provider: the console expires that session by the id it bound (the answer proves nothing and retires nothing), files the intent-keyed review naming it, and leaves the intent standing until the operator resolves it. A first purchase's subscription that names the bound intent and is refused for anything (price, currency, plan, band, terms, quantity) files the same review, after expiring the bound session if it is still open; a subscription naming another intent, or an intent the operator retired, is refused and settled. |
| +- **Checkout Session**: created with `currency` and `adaptive_pricing[enabled]=false`, and with `currency`, `price_book` in the session and subscription metadata. A purchase with no book (only a console with no book configured opens one, and an intent from before books is replayed as that build sent it) is asked for exactly as before: no currency, Adaptive Pricing on. Every session the console creates, replays, lists, reads back or binds passes one check against its intent; one that is this intent's and not its purchase (another currency, a conversion, Adaptive Pricing) is expired and the intent retired, and one already paid is left for a person with a review filed once. A subscription whose price is not the configured one for its stored (book, plan, band, currency), or that the provider reports in another currency, is an identity mismatch. Once an intent is bound, a read of its bound session that names another account, customer, intent or mode is a contradiction at the provider: the console expires that session by the id it bound (the answer proves nothing and retires nothing), files the intent-keyed review naming it, and leaves the intent standing until the operator resolves it: nothing records on an intent under an open review (the record's own transaction refuses), and the resolution either retires the intent (the default) or keeps it standing for a purchase corrected at the provider to record. A first purchase's subscription that names the bound intent and is refused for anything (price, currency, plan, band, terms, quantity) files the same review, after expiring the bound session if it is still open; a subscription naming another intent, or an intent the operator retired, is refused and settled. |
| - **Configuration**: `CONSOLE_BILLING_PRICE_BOOK` names the book and is required. `CONSOLE_STRIPE_PRICES` holds the 36 entries of the configured book, keyed `"<price_book>/<plan>/<band>/<currency>"`. Beside them it may hold the whole catalog of any other book this build has, and the twelve legacy keys `"<terms>/<plan>/<band>"`, each set all or none; those recognise subscriptions already bought and sell nothing. No price under two keys. Live mode with a draft book is refused. |
| - **Monthly calculation**: a month priced from a book is a version 2 snapshot: `currency`, `price_book`, and amounts under currency-neutral names. A version 1 snapshot is EUR, is read as it was written, and is never priced again. A stored calculation whose currency is not the month's assignment currency is refused. |
| - **Overage invoice**: the invoice and every invoice item are created in the calculation's currency and verified against it on every read. The currency is part of each request's parameters and so of its request hash; legacy EUR requests hash as they do today. |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review15.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review15.md| new file mode 100644 |
| index 000000000..55b5bdf21 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review15.md |
| @@ -0,0 +1,114 @@ |
| +**Fourteenth review, finding by finding** |
| + |
| +Platform references below are at `abd1b76e`, reviewed against `12e70d95` and the delta from `9574a798`. Site references are at `f291a9e`. Test counterfactuals are conclusions from source inspection: I ran no tests or mutations, made no changes, and used no network commands. |
| + |
| +| Fourteenth-review finding | Closed? | Would its regression test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1:** ordinary expiry cleanup removes a reviewed intent | **Closed for that path.** `internal/console/billingstore.go:454` refuses deletion while the intent’s refusal review is open. The broader lifecycle invariant still fails through recording: Finding 1 below. | **No.** The added pass at `internal/console/billingcurrency_test.go:3748` would remove the intent, failing the standing-intent and subsequent retirement-record assertions. | |
| +| **Opus P3-2:** subscription-only unknown terms retry indefinitely | **Closed for the reported standing, bound intent.** `internal/console/billingservice.go:856` routes it through `refuse()`. | **No.** The new unknown-terms case at `internal/console/billingcurrency_test.go:3573` requires 204 and a review; the former raw error produces 503. | |
| +| **Opus P3-3:** seven inclusions lack checks for bands B–D | **Closed.** Site `test/pricing-pricebook.test.mjs:106` now checks all ten inclusion slots across all bands. | The reported `0.75 GB` substitution now **fails**. Removing the added assertions lets it pass again. | |
| +| **Astra 1, P2:** reconciliation records after resolution retires its intent | **Closed.** First-purchase recording requires consumption of the exact standing intent in the recording transaction, at `internal/console/billingstore.go:758`. | **No.** `TestAResolutionDuringAReconciliationLeavesNothingRecorded`, at `internal/console/billingcurrency_test.go:3769`, requires a conflict and no recorded state, assignment or event. | |
| +| **Astra 2:** reviewed intent removed by cleanup | **Closed for cleanup**, as Opus P3-1; the broader invariant remains open. | **No**, for the extended cleanup regression. | |
| +| **Astra 3:** unknown-terms exception misses a permanent mismatch | **Closed for its reported case**, as Opus P3-2. | **No.** The subscription-only mutation leaves session metadata intact and reaches the intended branch. | |
| +| **Astra 4:** retired completions and subscriptions receive permanent retries | **Partly closed.** The no-replacement completion is settled at `internal/console/billingservice.go:1123`. Replacement-standing subscription cases remain: Finding 2. | **No** for the new completion assertion at `internal/console/billingcurrency_test.go:2944`. **Yes** for the remaining defect: the tests omit replacement-standing unknown-terms and `incomplete` events. | |
| +| **Astra 5:** a mismatched checkout-read ID still enables resolution | **Closed.** `internal/console/billingservice.go:824` stops before expiry or review. | **No.** The test at `internal/console/billingcurrency_test.go:3841` requires 503, no review and no expiry. | |
| +| **Astra 6:** incorrect per-currency slot values pass the site test | **Closed for both reported substitutions.** Site `test/pricing-pricebook.test.mjs:74` checks complete paid-price strings; `:78` checks Free in every currency. | The reported Community `$30/y per member` and Free `£22` substitutions now **fail**. Removing those assertions admits them again. Correct literals remain indistinguishable from derived values. | |
| + |
| +**Fourteenth-review claims** |
| + |
| +- **Opus claim 1 and Astra’s stale-comment claims:** closed. The comments at `internal/console/billingservice.go:246` and `:1073` now describe the read, expiry and review. |
| +- **Opus claims 2 and 5; Astra’s lifecycle claims:** partly closed. Expiry cleanup now preserves reviewed intents, but recording can still consume them before resolution. Not every retired event settles. |
| +- **Opus claim 3 and Astra’s price-terms mutation objection:** closed. The unknown-book case at `internal/console/billingcurrency_test.go:3574` independently reaches the price-terms refusal; an unknown band reaches `PriceFor`. |
| +- **Opus claim 4 and Astra’s rounded-rate objection:** the former `toFixed(2)` fallback is removed. The broader site assertions about complete numeric tokens and literal provenance remain overstated, as detailed below. |
| +- **Astra’s review-three/four mutation qualifications:** partly closed. The source acknowledges script-text checks and migration-test limits, but the explanation of the account-only deletion mutant remains wrong. |
| +- **Astra’s test-dependency observations:** real-time dependencies remain. They are not evidence of a newly introduced failure. |
| + |
| +The overlapping numbered findings in the two fourteenth reports were both P3. Their consequential difference was Astra’s additional **P2 recording-after-retirement race**, which the code substantiates and this transaction fix addresses. Opus’s description of recording while a review remains open as “benign” does not satisfy the explicit lifecycle invariant. On bound-session expiry, Opus’s reading was right: authority comes from the ID the console previously validated and bound. An unowned create or listing response supplies no equivalent authority. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P3 — Recording a corrected purchase still consumes an intent whose refusal review is open.** |
| + |
| + **Where:** `internal/console/billingstore.go:758`; resolution lookup at `:953`; reconciliation call at `internal/console/billingservice.go:1037`. |
| + |
| + **Scenario:** Complete a GBP checkout. Change the subscription’s band and matching price, causing the intent-keyed refusal review. Restore the original band and price, then deliver another subscription event **before resolving the review**. |
| + |
| + Reconciliation now passes. The recording transaction updates the account, writes its assignment, queues the entitlement and deletes the intent. Its new consumption check requires one deleted row, but does not require the absence of an open review. |
| + |
| + The review remains open without its intent. Resolution subsequently succeeds but finds nothing to archive, so no `billing_retired_intents` record is written. This can also happen while an operator is settling or refunding the reviewed purchase. |
| + |
| + **Smallest acceptable fix:** Make first-purchase consumption conditional on there being no open refusal review for that exact account and intent, within the recording transaction. Add the existing race fixture’s corrected-provider continuation **without** its resolution hook, asserting that nothing records and the intent remains standing. |
| + |
| + The new race test does not cover this ordering: it resolves the review before the recording transaction begins. |
| + |
| +2. **P3 — A replacement intent prevents some retired-subscription events from being settled.** |
| + |
| + **Where:** `internal/console/billingservice.go:804`, `:856`, `:878`; webhook response at `internal/console/billinghandlers.go:308`. |
| + |
| + **Scenario:** Intent A’s completed purchase receives a review because only its subscription’s terms were changed to an unknown version. Resolve A, then open replacement intent B. Deliver another event for A’s subscription. |
| + |
| + Because B stands, the early retirement lookup is skipped. The unknown-terms exception then returns the raw error because B is not A. The webhook answers **503**, although A is archived and can never record. |
| + |
| + An old subscription reporting `incomplete` similarly returns `errBillingPending` before reaching the different-intent refusal. |
| + |
| + **Smallest acceptable fix:** For an incoming subscription other than the account’s already-recorded subscription, check its account-scoped retired intent before purchase validations, independently of whether another intent stands. Test both unknown terms and `incomplete` while replacement B remains untouched. |
| + |
| +3. **P3 — The site’s numeric-token check still accepts the suffix of a grouped number.** |
| + |
| + **Where, site:** `copy-review/pricing/build.mjs:147`, `:154`, `:155`; `test/pricing-pricebook.test.mjs:101`. |
| + |
| + **Scenario:** Change Free’s approved storage figure from `250 MB of storage` to `1,250 MB of storage`. The check still finds `250 MB` after the comma. Likewise, `1,100 awake hours a month` satisfies the check for `100 awake hours`. |
| + |
| + The new right-hand boundary rejects `10,000` when checking `10`, but the left-hand boundary still treats a comma as a valid separator. Free’s inclusions are rendered directly, without the export-derived equality checks added for paid inclusions. |
| + |
| + **Smallest acceptable fix:** Parse and compare the complete numeric quantity, or exclude grouping commas at both numeric boundaries. Add both counterexamples. |
| + |
| + The pinned page currently contains the correct figures; this finding concerns the promised guard against publishing contradictory figures. |
| + |
| +**Other requested paths: no additional finding** |
| + |
| +- **GBP end to end — A.** `GET ?country=IN¤cy=gbp` quotes band D in GBP. Personal is **£7.70 annually**, with **29.75 awake hours**, **0.7 GB storage** and **0.7 GB transfer**; checkout also requires India’s postal information. The handler compares the signed quote with the effective purchase currency before calling `startBillingCheckoutQuoted` (`internal/console/billinghandlers.go:71`, `:102`, `:117`). The service verifies the exact GBP provider price and stores the book/currency on the intent (`internal/console/billingservice.go:482`, `:514`, `:576`). Binding and reconciliation check purchase identity before recording. History carries those terms into `AllowanceForMonth`, version-2 calculations, invoices, recovery and quantity updates (`internal/console/billinginvoicestore.go:529`; `internal/plans/month.go:167`; `internal/console/billingcalculation.go:174`; `internal/console/billinginvoice.go:901`, `:1024`; `internal/console/billingquantity.go:1016`). I found no cross-currency computation, conversion or historical repricing. |
| + |
| +- **Form, quote, cookies and signatures — B/C/M/O/Q/U/Z.** The HMAC derives its secret from the billing secret and covers session token, account, book, country, band, currency, choice flag and member count (`internal/console/billingpage.go:446`). Disclosed form values cannot forge another tuple. Another session, account, book or rotated token invalidates it. An older tab may reuse its own valid quote, but buys its own displayed terms, with membership checked again. All offered plans were displayed; quantity follows signed membership and plan minimums; postal affects tax. Present currency fields must contain exactly one sold lowercase value; empty, repeated, padded and unsupported values fail on both doors (`internal/console/billinghandlers.go:176`). An absent field uses the country default on a book console and EUR on a legacy console, still subject to quote comparison. The quote door only constructs an encoded account-local redirect. Held currency, including legacy EUR, overrides suggestions. Present-empty cookies clear; absent parameters fall back; invalid parameters leave existing cookies unchanged, so an older preference can reappear on a later parameter-absent GET. Cookies cannot override the submitted form. |
| + |
| +- **Ownership, refusal and races — D/L/R/W/Y/AA/DD/EE/FF/HH–LL/NN.** `ownSession` checks mode, both account identifiers, intent, known customer and bound ID (`internal/console/billingservice.go:272`). Create, replay and listing responses cannot authorize actions on foreign sessions. Expected-intent binding and the pass use the account of the intent being processed. Bound-ID expiry is justified even when returned metadata contradicts ownership; the returned expiry object is not treated as retirement proof. Read/expiry failures stop review filing and produce webhook retries. A mismatched refusal-read ID now does likewise. Intent-keyed filing prevents duplicate open occurrences. Provider-proof retirement separately checks account metadata, customer, intent, mode, expired status and absence of payment/subscription (`internal/console/billingstore.go:441`). Unowned, unbound sessions remain open at the provider; declining to expire them is the correct call. |
| + |
| +- **Recording and retirement — S/V/BB/GG/OO/RR–TT.** Every production first-purchase recording reaches the new transactional consumption check; the older convenience wrappers have no production callers. A missing or replaced intent rolls back account, assignment, event and entitlement writes together. Already-recorded subscriptions and renewals do not require a consumed intent; revision checks protect concurrent changes. The binder’s fast path merely returns an already-recorded subscription. Resolution archives and deletes only the named account/intent and settles only its pending quantity events. Late review filing cannot reopen a retired intent. References survive both readers and the operator API. The distinct-session replay now detects unwanted recording; the separate no-replacement test asserts 204. Checkout recovery, pass expiry and account-deletion recovery use the guarded retirement helpers; stillborn cleanup applies before checkout creation. The remaining violations are Findings 1–2, not an unsafe account-wide resolution delete. |
| + |
| +- **History barrier and rollout — E/H/N/T.** The trigger matches the positive paid span used by `AllowanceForMonth`: active, non-Free, beginning before the clipped end, which is the earliest successor assignment, paid-through instant or month end (`internal/console/billingstore.go:208`; `internal/plans/month.go:146`). Successors include both tables and higher same-second revisions. Same-second supersession contributes no span; Free/inactive rows terminate predecessors. Renewal and quantity dating can move `effective_at`; assignment `period_end` remains the subscription’s paid-through endpoint. I found neither a valid older-build version-1 escape nor an erroneous refusal of this build’s legacy-only month. Replacement-trigger creation precedes transactional retirement of the old name (`internal/console/billingstore.go:828`). Missing replacement preserves the old trigger; interruption can temporarily leave both, not neither; another successful start is idempotent. This does not make unsupported binary rollback safe. |
| + |
| +- **Amounts, configuration and quantity history — F/G/J/X/CC.** Console inclusions divide pooled allowances by the actual charged quantity, including unavailable Personal’s fallback (`internal/console/billingpage.go:395`). Current shares are exact across supported quantities, and band A is asserted against the approved sentences. Book validation checks the complete matrix, rounding and overflow, country mappings, duplicate keys and legacy EUR equality. Catalogs of additional supported books are all-or-none, with unique provider price IDs. Provisioning accepts at least the selected book’s 36 keys and the same own-only currency-options shape. The export contains commercial definitions, not customer data or secrets. All three quantity-outcome admission checks use `quantityOutcomesSQL`; startup replaces a changed guard, and no sweep clears history gaps. Pre-history and clock-rollback holds remain. |
| + |
| +- **Pricing site — K/MM/QQ.** The site’s book and the platform public export are byte-identical by SHA-256. Current rates, all paid inclusion bands, countries and country defaults agree. Draft gates require exactly `PRICEBOOK_ALLOW_DRAFT=1`; the marker is first-line and the ordinary build invokes the check. Links always contain both parameters, forwarding currency only when chosen. A locale-only display can differ from the console’s eventual country default because it intentionally carries no currency choice; the console then supplies the signed purchase quote. |
| + |
| +**Claims the code contradicts** |
| + |
| +- **“The intent stands until resolution” and “resolution alone retires it” remain false.** See Finding 1 and `docs/superpowers/specs/DISPOSITIONS-multi-currency.md:286`, `:291`, `:296`; design `:152`. |
| + |
| +- **“Every later event is settled” and retirement preceding unrecoverable validation remain false while a replacement stands.** See Finding 2 and dispositions `:286`, `:293`. |
| + |
| +- **The site test does not establish literal provenance.** At site `test/pricing-pricebook.test.mjs:189`, its title and comment claim a literal substituted into any slot fails. A literal equal to that slot’s correct current value passes. The assertions establish output equality. A changed-book reconstruction or source check is needed for the stronger claim. |
| + |
| +- **“Whole token” remains incomplete.** Site `copy-review/pricing/build.mjs:142` conflicts with Finding 3. The disposition’s narrower description of the new right-hand grouping-comma check is accurate. |
| + |
| +- **The review-four deletion-mutant explanation is wrong.** Dispositions `:108` attributes indistinguishability to the table having one intent per account. A review for old intent A while newer B stands is a distinguishing history. The current **delete-only** mutant is redundant because the preceding exact-intent lookup at `internal/console/billingstore.go:953` already gates the deletion within the transaction. |
| + |
| +- **The provisioning instructions still say twelve keys.** `implementation/carloku-paid-launch/stripe-provision.sh:379` prints `CONSOLE_STRIPE_PRICES='{...12 keys...}'`, while this verifier requires the selected book’s 36. |
| + |
| +- **Test evidence has limits.** The new race test checks state, assignments and events, but does not directly inspect the entitlement queue; rollback follows from the transaction. The original late-replay helper at `internal/console/billingcurrency_test.go:2874` still discards HTTP status. Fake expiry and webhook freshness use real time (`internal/console/billingservice_test.go:134`; `internal/console/billingcurrency_test.go:2875`). I found no new test-order, temporary-root or shared-cookie-jar failure. Historical mutation-run counts cannot be established from source; script-text assertions do not execute provisioning checks, and successful migration does not test interruption ordering. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +1. **Catalog and purchases:** Verify all 36 price identities, amounts, own-only currency options, quantities, tax behavior and disabled Adaptive Pricing. Complete USD/EUR/GBP purchases, including IN/GBP and Community GBP bands B/D. Each must retain its chosen currency throughout. |
| + |
| +2. **Independent identity changes and event ordering:** Change price, subscription/item currency, quantity and purchase metadata independently while leaving session metadata intact. Deliver completion and subscription events in both orders. Valid purchases must record once; attributed mismatches must produce one referenced review. |
| + |
| +3. **Expiry versus completion:** Exercise bound-ID expiry after metadata changes, failed reads, failed expiry and payment completing during expiry. Establish the returned object shapes, URL unusability and idempotency behavior. Failures must retry without falsely enabling resolution. |
| + |
| +4. **Reviewed and retired purchases:** Correct provider metadata while a review remains open; resolve during reconciliation’s customer read; replay retired completion and subscription events with no replacement and with a replacement, including unknown terms and `incomplete`. After the fixes, reviewed purchases must remain held, retired purchases must never activate, and their events must settle with 204. |
| + |
| +5. **Overage invoices and recovery:** Create, recover, finalize and pay invoices in every currency. Verify invoice/item/line currencies, exact allocations, floors, discounts, tax, house waivers and request-hash recovery. Replay legacy EUR request shapes. |
| + |
| +6. **Subscription lifecycle and quantities:** Exercise member increases, proration, renewal, cancellation, quantity fencing and allowance dates. Establish customer-currency restrictions after cancellation and while earlier invoice items remain outstanding. |
| + |
| +**Verdict: accept with fixes.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md| index 1637591f2..0e3b85a8a 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -105,7 +105,7 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## Review four (at 8958392b; Opus accepted with fixes, Astra rejected) |
| -Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency-review4.md`. Both found the third round's code changes present and no path that stores or sends money in a currency other than the purchase's (Astra marked ownership, the crafted form and the trigger migration partly closed, which its findings were about); the disputed trigger reading went Astra's way. Every code finding of either that is marked fixed below has a test that fails without it, with three qualifications review ten named: the provisioning script's checks are tested by reading the script's text, the trigger's create-then-drop order is tested by a migration that succeeds (not by one that would fail in the other order), and the resolution's delete scoped by account alone cannot be told from the delete by intent: the intents table is keyed by account, so one account has one intent and no fixture can distinguish the two (the review-six row says the same of its surviving mutant); eight mutants, one per fix, were killed in a scratch copy at the time (the runs are not reproducible from this file); two items are left open and say so. |
| +Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency-review4.md`. Both found the third round's code changes present and no path that stores or sends money in a currency other than the purchase's (Astra marked ownership, the crafted form and the trigger migration partly closed, which its findings were about); the disputed trigger reading went Astra's way. Every code finding of either that is marked fixed below has a test that fails without it, with three qualifications review ten named: the provisioning script's checks are tested by reading the script's text, the trigger's create-then-drop order is tested by a migration that succeeds (not by one that would fail in the other order), and the resolution's delete scoped by account alone is not told from the delete by intent by any test: the exact-intent lookup earlier in the same transaction already gates the delete, so the mutant is redundant rather than wrong (a review of an old intent while a newer one stands is a distinguishing history for the lookup, which the resolution test covers, not for the delete alone); eight mutants, one per fix, were killed in a scratch copy at the time (the runs are not reproducible from this file); two items are left open and say so. |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| @@ -296,3 +296,18 @@ Reports: `OPUS-REVIEW-multi-currency-review14.md`, `CODEX-FINDINGS-multi-currenc |
| | Claims (both): `intentSession`'s "without reading it"; `bindBillingCheckoutFor`'s "reviews nothing"; two test comments; the design's "stands until resolved" and "terms"; the review-twelve bracket on the price-terms case; the review-thirteen rows on the site fallbacks and on `refuse()`'s expiry. | Corrected in place; the design now holds again by the fixes above. | this file, `billingservice.go`, tests | | |
| Open, as both list: everything that needs Stripe test mode (both lists, with the resolution-during-reconciliation ordering, the expiry races, a retired session's completion and a terms edit added this round), the two unreviewed system strings, the operator's door for a quantity-history gap, a listing of rejected webhook events. The fifteenth review reads this commit. |
| + |
| +## Review fifteen (reviewed `abd1b76e`; Opus accepted with fixes, Astra accepted with fixes) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review15.md`, `CODEX-FINDINGS-multi-currency-review15.md` (in the tree from this commit). Both found the fourteenth round's findings closed, each with a test that fails without its fix, Astra's P2 included. Both found the one exception left to the lifecycle invariant: a purchase corrected at the provider recorded, and consumed its intent, while the intent's review was still open, so the resolution then retired nothing. Opus would have had the resolution report it; Astra, and the supervision ruling, had the record refuse. The record refuses. Fixed, each with a test that fails without it; three mutants killed in a scratch copy (the record's open-review clause removed; the keep outcome ignored; the retirement check back behind "no intent stands"): |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P3-1, Astra 1 (P3): a corrected purchase recorded under an open review. | Fixed in the record's own transaction: for a first purchase the intent is consumed only if no refusal review of it is open; otherwise nothing is recorded (`errBillingConflict`, the provider tries again) and the intent stands. The operator's resolution gained an outcome: `retire` (the default: the money is settled, the intent retired with its record) or `keep` (the purchase was corrected at the provider: the review is resolved and the intent stands, so the next event records it). The API answers with `retired`. | `billingstore.go` `applyBillingStateDated`, `resolveBillingReview` (`Outcome`); `billingoperator.go` | `TestACorrectedPurchaseWaitsForTheOperatorUnderItsReview` (the corrected event is 503 and records nothing; the intent stands; `keep` resolves and the next event records; an unknown outcome is 400) | |
| +| Astra 2 (P3): a retired intent's subscription events were still retried while a replacement intent stood (the retirement check sat behind "no intent stands"; unknown terms and an incomplete status then returned raw). | Fixed: the retirement check runs for every subscription that is not the recorded one, before any validation, whatever intent stands. | `billingservice.go` `reconcileBillingSubscription` | `TestARetiredSubscriptionSettlesBesideAReplacementIntent` (unknown terms, then `incomplete`, with the replacement standing: 204 each; the replacement untouched) | |
| +| Astra 3 (P3): the site's figure-token check took a grouping comma as a left boundary (250 MB in 1,250 MB). | Fixed (site `a7283e7`): the token must not be part of a larger number on either side, commas included; the check lives in `hack/token.mjs` with its counterexamples as a test. | site `hack/token.mjs`, `test/pricing-pricebook.test.mjs` | | |
| +| Opus claim 2: the unknown-terms comment claimed the intent's terms are known. | Corrected: the intent's terms are not compared there; an intent made by a newer build before a rollback is reviewed rather than retried, as the binder reviews its session. | `billingservice.go` | | |
| +| Opus claim 3: the early retirement check was not told from the later one. | Fixed: a retired subscription's event with unknown terms after the resolution is settled (the later check would not be reached). | tests | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` | |
| +| Astra claims: the review-four deletion-mutant explanation; the provisioning script's "12 keys"; the design and this file's lifecycle sentences. | Corrected: the delete-only mutant is redundant because the exact-intent lookup in the same transaction gates it (not because one account has one intent); the script says the book's 36 keys; the design states the guard and the two outcomes. | this file, `stripe-provision.sh`, the design | | |
| + |
| +Open, as both list: everything that needs Stripe test mode (both lists, with a corrected purchase under an open review and the `keep` outcome added this round), the two unreviewed system strings, the operator's door for a quantity-history gap, a listing of rejected webhook events; the site test establishes output equality per slot, not literal provenance (stated in its title's qualification). The sixteenth review reads this commit. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review15.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review15.md| new file mode 100644 |
| index 000000000..3eefe3aac |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review15.md |
| @@ -0,0 +1,105 @@ |
| +# Fifteenth review of `feat/carloku-multi-currency-20261001` at `abd1b76e` (base `12e70d95`) |
| + |
| +**Verdict: accept with fixes.** Every finding from the fourteenth round is closed for the case it reported. Each has a test that fails without its fix. Astra's P2 was a real defect and it is now closed in the database. One P3 remains: a purchase can still be recorded while its review is open, which contradicts the lifecycle invariant the dispositions now state. There is also one comment claim to correct. This round changes nothing in how money is computed, sent to Stripe or stored. |
| + |
| +I read source with `git show` only, and the site repository at `f291a9e`. I ran nothing and modified nothing. My scratch copies under `$TMPDIR` are left for the session's cleanup. |
| + |
| +## The fourteenth review's findings |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Astra 1 (P2)**: a reconciliation paused on a provider read could record a purchase after the operator retired its intent | **Closed.** For a first purchase, `consumeIntent = state.Subscription != id` (`billingservice.go:1037`). The record transaction deletes the exact intent and returns `errBillingConflict` unless exactly one row went (`billingstore.go:757-767`). The rollback takes the assignment, entitlement queue, event row and quantity refresh with it. Reconcile is the only production caller of `applyBillingStateDated`; `applyBillingState` and `applyBillingStateAt` are used only by tests and pass `false`. | **No.** `TestAResolutionDuringAReconciliationLeavesNothingRecorded` resolves inside the customer read. The resolution changes no revision, so without the row count the record commits and returns nil. The test requires a conflict. | |
| +| **Opus P3-1 / Astra 2**: the pass retired the intent under its open review | **Closed** for `retireIntent` (`billingstore.go:454-456`), which covers all three retire paths: click, `refuseSession` and the pass. `discardUnreadyIntent` cannot meet a review, because an unready intent never had a session. | **No.** Without the clause the pass's `expireResolvedIntent` deletes the intent and the test's next `billingCheckoutIntent` fails. | |
| +| **Opus P3-2 / Astra 3**: a terms version edited on the subscription alone gave 503 forever | **Closed** (`billingservice.go:856`). | **No.** The case "a terms version this build does not know" needs 204 and a review in all three orders. The old raw error is a 503. | |
| +| **Astra 4**: a retired session's completion, and the retirement check placed after validations | **Closed** in the binder (`:1119-1128`) and by the early check in reconcile (`:804-810`). | **No** for the binder. With no replacement intent standing, the binder returns `ErrNoRows`, which is a 503, and the test needs 204. The early reconcile check has no test that tells it apart from the later one at `:893`. Only an edited-terms subscription after resolution would distinguish them. | |
| +| **Astra 5**: `refuse()` filed a review on a read answered with another session | **Closed** (`:824-830`). | **No.** The old code skipped the expiry but still filed and answered 204. The test needs 503 and no review. | |
| +| **Opus P3-3 / Astra 6**: site inclusions in bands B–D, prefix matches, Free, the `toFixed(2)` rate form, `10` found inside `10,000` | **Closed** (site `f291a9e`). Every band of all ten inclusions is recomputed from the export. Every plan, band and currency slot is checked for equality including its suffix, and Free in all three currencies. Rates are spelt exactly and floors as prices. `must` now refuses `,[0-9]` after a token. | **No.** `$30/y per member` now fails the equality check, and a band-D figure typed into `build.mjs` fails the recomputation. | |
| +| **Claims** (both reviewers) | **Corrected:** `intentSession`'s comment, the binder's comment, the test comment at `:3373`, the review-twelve bracket and the review-thirteen site row. **Still contradicted:** the design's "stands until resolved" (P3-1 below). | n/a | |
| + |
| +**Where the reviewers disagreed.** Astra rejected on its P2; Opus accepted with fixes and did not report the race. At `9574a798` the code shows Astra was right. Resolution (`billingstore.go:942-980`) took no service lock and left `billing_accounts.revision` unchanged. The record's intent `DELETE` counted no rows. So a reconcile paused in `GetCustomer` would have recorded and queued entitlement for a purchase the operator had just retired. The outcome is access granted after an operator's refund, so P2 is a fair severity. |
| + |
| +## Findings |
| + |
| +### P3-1. A purchase can be recorded while its intent's review is open, and the resolution then retires nothing without saying so |
| + |
| +- **Where:** `billingstore.go:757-767`. The consuming `DELETE` has no open-review clause, unlike `retireIntent` at `:454-456`. Then `billingstore.go:960-980`: the resolution resolves even when the intent is gone. |
| +- **Scenario:** |
| + 1. A GBP Personal intent completes. |
| + 2. Its subscription is edited to band D at the provider. Reconcile's `refuse()` files the review. |
| + 3. The metadata is put back at the provider. The next event passes every check, and `applyBillingStateDated(consumeIntent=true)` deletes the intent and records the subscription while the review is still open. This is the race test's own fixture without its hook. |
| + 4. The operator refunds at Stripe and resolves with "refunded". The resolution finds no intent, writes no `billing_retired_intents` row, and succeeds. |
| +- **Result:** |
| + - The account keeps an active paid subscription and entitlement the operator believes they retired. |
| + - The design (`:152`) and dispositions say "an intent under an open review stands, whatever its session's state, until the operator resolves it". The record path is the one exception left. |
| + - Damage is limited: the open review holds the account's invoices (`billinginvoicestore.go:167`), and a later cancellation at Stripe would record. |
| +- **Smallest acceptable fix:** make the resolution of `billing_checkout_currency_refused` report when no intent stands, with a distinct error or result naming the account's recorded subscription, so the operator learns the purchase was recorded. State the exception in the design and the dispositions. |
| + - The alternative is the `retireIntent` clause on the consuming delete: refuse to record under an open review. That leaves the operator no way to accept a corrected purchase, and a paid customer's events would be settled after resolution. I would not take it without an "accept" door. |
| + - Add a test: review, then correction, then a record, then a resolution. Assert what the operator is told. |
| + |
| +## The added sections |
| + |
| +**RR. The record consumes the exact standing intent.** |
| +- **Every first purchase is behind it.** Reconcile is the only production recorder. The binder only binds, and its fast path returns already-recorded state. |
| +- **Renewals and recorded subscriptions are not refused by it.** For those, `state.Subscription == id`, so `consumeIntent` is false. |
| +- **A concurrent record of the same subscription** fails the revision check first. |
| +- **A replayed event id** returns nil at `:584` before consuming. That is correct, because its first application consumed. |
| +- **A refused record leaves nothing half-done.** Everything, including the location review, the entitlement queue, the event row and `refreshQuantityChangeTx`, is inside the transaction. The webhook answers 503 (`billinghandlers.go:308`). The retry finds no intent standing and the retirement record, and settles with 204. The race test covers that last step. |
| + |
| +**SS. Every path that reads or retires an intent.** |
| + |
| +| Path | Under an open review | |
| +|---|---| |
| +| Click: previous intent | Guarded, via `expireBillingCheckout` → `retireIntent`. The buyer is shown "conflict" until the operator resolves. | |
| +| Click: resumed intent | Guarded, via the same call. | |
| +| `refuseSession` | Guarded. | |
| +| Pass `expireResolvedIntent` | Guarded, and the conflict is ignored. | |
| +| `discardUnreadyIntent` | Unreachable: an unready intent has no session, so no review. | |
| +| Resolution | The one intended retirement. | |
| +| Record consumption | **The exception: P3-1.** | |
| + |
| +- A retired intent never records: the record's row count, the early retired check (`:804`), and the binder's check (`:1119`) see to it. |
| +- Its events settle. One gap: with a replacement intent standing, a retired subscription whose terms were edited still gets a raw 503 at `:856`, because `pending` names the replacement. It settles once the replacement records or expires, within Stripe's retry window. Acceptable. |
| + |
| +**TT. The resolution takes no service lock.** |
| +- **The record:** now serialized by SQLite and the row count. |
| +- **The binder's bind `UPDATE`:** zero rows, so a conflict. |
| +- **`fileCheckoutRefusedReview`:** checks that the intent stands inside its transaction. |
| +- **A click:** finds the intent gone, so a conflict, or begins a new intent. It cannot resume a payable URL, because every session under review was expired first (`refuse()` at `:831` and `ownBoundSession`) or is already complete. |
| +- **The pass's expiry branch:** conflict, ignored. |
| +- **The quantity worker:** `checkout_refused_resolved` is in `quantityOutcomesSQL`. |
| + |
| +Nothing is unsafe. |
| + |
| +**NN, OO and PP, after this delta.** |
| +- **NN:** `refuse()` acts only on `pending.SessionID`, and a foreign answer is now a retry. |
| +- **OO:** retirement stays keyed by account and intent, and no path settles another account's retirement. |
| +- **PP:** the only 503 left for a first purchase that will never succeed is the replacement-standing case above. |
| + |
| +**MM/QQ/K. The site at `f291a9e`.** |
| +- Every slot in every currency is pinned. A literal in any slot fails, and Free is included. |
| +- Whole-token matching now also refuses grouping commas. |
| +- The test's `figure` rounds with `toFixed(3)` while the console's `formatFigure` truncates (`billingpage.go:467-490`). They agree only because the build's exactness check refuses inexact shares, which is true of the current book. |
| +- The links and the draft gate are unchanged. |
| + |
| +**Unchanged sections.** The delta touches only `billingservice.go`, `billingstore.go` and tests. A–M and O–LL stand as rounds ten to fourteen traced them. One GBP purchase from `?country=IN¤cy=gbp` still has no conversion or re-pricing anywhere in its chain. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **Design `:152` and the dispositions' review-fourteen preamble.** "An intent under an open review stands … until the operator resolves it" is false for the record path (P3-1). |
| +2. **`billingservice.go:851-855`.** "That intent's own terms are known here" is not checked. An intent made by a newer build before a rollback has terms this build does not know, and its legitimate subscription is reviewed rather than retried. The binder's `intentSession` (`:255-257`) already reviews the same session, so the behaviour is consistent. Correct the comment rather than the code. |
| +3. **Dispositions, review fourteen, Astra 4 row.** "Reconcile checks the retirement first" is true, but no test tells the early check apart from the later one at `:893`. Say so, or add an edited-terms subscription event after resolution with no replacement standing. |
| + |
| +I found no contradiction in the six mutation claims for this round. Each mutant is killed by the test I traced above. |
| + |
| +## Verify in Stripe test mode before enabling |
| + |
| +All earlier lists stand. Add: |
| + |
| +1. **A refused purchase, its metadata corrected at the provider, then the next event.** Confirm it records under the open review, and see what the operator then sees on resolution (P3-1). |
| +2. **The pass under an open review with the session expired.** The intent stays, and each click answers "conflict" until resolution. |
| +3. **After a resolution with no replacement intent**, re-deliver `checkout.session.completed` and an edited-terms `customer.subscription.updated`. Expect one rejected-events row and 204 each, and that Stripe stops retrying. |
| +4. **A refusal whose session read Stripe answers inconsistently, if Stripe can be made to.** Expect 503 and no review until a consistent read. |
| + |
| +The resolution-during-reconcile ordering is local and cannot be produced against Stripe. The race test is its only evidence. |
| + |
| +**Verdict: accept with fixes** (P3-1 and the three claims). |
diff --git a/implementation/carloku-paid-launch/stripe-provision.sh b/implementation/carloku-paid-launch/stripe-provision.sh| index eb56f6a17..132a44bbc 100755 |
| --- a/implementation/carloku-paid-launch/stripe-provision.sh |
| +++ b/implementation/carloku-paid-launch/stripe-provision.sh |
| @@ -376,7 +376,7 @@ print_provision() { |
| print_check() { |
| header |
| note "Read-only. Set these first (ids are not secret):" |
| - note " export CONSOLE_STRIPE_PRICES='{...12 keys...}' PORTAL_CONFIG_ID=bpc_..." |
| + note " export CONSOLE_STRIPE_PRICES='{...the book's 36 keys...}' PORTAL_CONFIG_ID=bpc_..." |
| note " export HOUSE_COUPON_ID=carloku-house-100-forever PUBLIC_COUPON_ID=carloku-public-50-forever" |
| printf 'LIVE=%s\nEXPECTED_ACCOUNT=%s\nTERMS=%s\nBOOK=%s\n' "$LIVE_JSON" "$account" "$TERMS" "$BOOK" |
| printf ': "${CONSOLE_STRIPE_PRICES:?set CONSOLE_STRIPE_PRICES}" "${PORTAL_CONFIG_ID:?set PORTAL_CONFIG_ID}" "${HOUSE_COUPON_ID:?set HOUSE_COUPON_ID}" "${PUBLIC_COUPON_ID:?set PUBLIC_COUPON_ID}"\n\n' |
diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go| index 713196b98..b05a63e8e 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -2944,6 +2944,18 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| if code := f.webhookPost(t, fx, "evt_after_resolution_done", "checkout.session.completed", intent.SessionID); code != http.StatusNoContent { |
| t.Fatalf("the retired session's completion after the resolution: %d; want it settled with 204, not retried", code) |
| } |
| + // A retired subscription whose terms were then edited to a version |
| + // this build does not know: settled before that validation (review |
| + // fifteen: the early retirement check, told from the later one). |
| + f.mu.Lock() |
| + f.meta["terms_version"] = "2031-01-01-v9" |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_after_resolution_terms", "customer.subscription.updated", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("a retired subscription with unknown terms after the resolution: %d; want it settled with 204", code) |
| + } |
| + f.mu.Lock() |
| + f.meta["terms_version"] = plans.CurrentTermsVersion |
| + f.mu.Unlock() |
| var retiredSession, retiredCurrency string |
| if err := fx.st.db.QueryRow(`SELECT session_id, currency FROM billing_retired_intents WHERE account_id=? AND intent_id=?`, acct.ID, intent.ID).Scan(&retiredSession, &retiredCurrency); err != nil || retiredSession != intent.SessionID || retiredCurrency != "gbp" { |
| t.Fatalf("the retired intent's record: %q %q %v", retiredSession, retiredCurrency, err) |
| @@ -3860,3 +3872,123 @@ func TestARefusalWhoseSessionReadIsAnsweredWithAnotherFilesNothing(t *testing.T) |
| t.Fatalf("%d expiries; want none on another session's id", f.expireCalls) |
| } |
| } |
| + |
| +// A refused purchase whose metadata is corrected at the provider does not |
| +// record while its review is open: the record's own transaction refuses |
| +// (nothing recorded, the intent standing), and the operator decides. A |
| +// resolution that keeps the intent lets the next event record it; the |
| +// default retires it. (Review fifteen, both reviewers, and the ruling that |
| +// the activation guard is transactional.) |
| +func TestACorrectedPurchaseWaitsForTheOperatorUnderItsReview(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + intent, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + sessionMeta := map[string]string{} |
| + f.mu.Lock() |
| + for k, v := range f.meta { |
| + sessionMeta[k] = v |
| + } |
| + f.sessionMeta, f.sessionStatus = sessionMeta, "complete" |
| + f.meta["band"], f.subscriptionPrice = plans.BandD, currencyPriceID(plans.PlanPersonal, plans.BandD, plans.GBP) |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_corr_sub", "customer.subscription.created", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the mismatched subscription event: %d", code) |
| + } |
| + // Corrected at the provider: the next event passes every check and is |
| + // still refused by the record's transaction while the review is open. |
| + f.mu.Lock() |
| + f.meta["band"], f.subscriptionPrice = intent.Band, intent.Price |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_corr_sub_2", "customer.subscription.updated", "sub_paid"); code != http.StatusServiceUnavailable { |
| + t.Fatalf("the corrected subscription event under the open review: %d; want 503 (nothing recorded, try again)", code) |
| + } |
| + if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "" { |
| + t.Fatalf("state %+v; want nothing recorded under the open review", state) |
| + } |
| + if standing, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || standing.ID != intent.ID { |
| + t.Fatalf("intent %+v %v; want it standing", standing, err) |
| + } |
| + // The operator resolves keeping the intent: the next event records. |
| + token := billingOperatorToken(t, fx, "ops@keymail.dev", "code-billing-op-corrected") |
| + status, body := postJSON(t, fx.ts.URL+"/api/billing/reviews/resolve", map[string]any{"account": acct.Sqid, "subscription": intent.ID, "reason": billingCheckoutRefusedReason, "occurrences": 1, "note": "corrected at the provider", "outcome": "keep"}, token) |
| + if status != http.StatusOK || body["resolved"] != true || body["retired"] != false { |
| + t.Fatalf("the resolution's answer: %d %v; want resolved and the intent kept", status, body) |
| + } |
| + if standing, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || standing.ID != intent.ID { |
| + t.Fatalf("after keeping: intent %+v %v; want it standing", standing, err) |
| + } |
| + if code := f.webhookPost(t, fx, "evt_corr_sub_3", "customer.subscription.updated", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the corrected subscription event after the review was resolved: %d", code) |
| + } |
| + if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "sub_paid" { |
| + t.Fatalf("state %+v; want the corrected purchase recorded once the operator kept the intent", state) |
| + } |
| + if _, err := fx.st.billingCheckoutIntent(acct.ID); err == nil { |
| + t.Fatal("the intent stands after the record consumed it") |
| + } |
| + // An outcome that is neither is refused. |
| + if status, _ := postJSON(t, fx.ts.URL+"/api/billing/reviews/resolve", map[string]any{"account": acct.Sqid, "subscription": intent.ID, "reason": billingCheckoutRefusedReason, "occurrences": 1, "note": "x", "outcome": "shrug"}, token); status != http.StatusBadRequest { |
| + t.Fatalf("an unknown outcome: %d; want 400", status) |
| + } |
| +} |
| + |
| +// A retired intent's subscription events are settled whether or not a |
| +// replacement intent stands, before any validation whose retry could never |
| +// record: an unknown terms version, and an incomplete status. (Review |
| +// fifteen, Astra 2.) |
| +func TestARetiredSubscriptionSettlesBesideAReplacementIntent(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + first, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + f.mu.Lock() |
| + f.sessionStatus, f.sessionCurrency, f.subscriptionPrice = "complete", "eur", first.Price |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_rep_done", "checkout.session.completed", first.SessionID); code != http.StatusNoContent { |
| + t.Fatalf("the completion in another currency: %d", code) |
| + } |
| + if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.resolveBillingReview(billingReviewResolution{AccountID: acct.ID, Subscription: first.ID, Reason: billingCheckoutRefusedReason, Occurrence: 1, Actor: "paul@keymail.dev", Note: "refunded"}, f.now.Add(10*time.Minute)); err != nil { |
| + t.Fatalf("resolve: %v", err) |
| + } |
| + // A replacement purchase opens and stands. |
| + oldMeta := map[string]string{} |
| + f.mu.Lock() |
| + for k, v := range f.meta { |
| + oldMeta[k] = v |
| + } |
| + f.sessionStatus, f.sessionCurrency, f.sessionID = "open", "", "cs_test_second" |
| + f.mu.Unlock() |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now.Add(20*time.Minute)); err != nil || dest == "" { |
| + t.Fatalf("the replacement purchase: dest %q err %v", dest, err) |
| + } |
| + second, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + if second.ID == first.ID { |
| + t.Fatal("fixture: no replacement intent") |
| + } |
| + // The retired subscription's events, with the replacement standing: |
| + // unknown terms, then an incomplete status. |
| + f.mu.Lock() |
| + f.meta = oldMeta |
| + f.meta["terms_version"] = "2031-01-01-v9" |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_rep_terms", "customer.subscription.updated", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the retired subscription with unknown terms beside a replacement: %d; want 204", code) |
| + } |
| + f.mu.Lock() |
| + f.meta["terms_version"], f.subscriptionStatus = plans.CurrentTermsVersion, "incomplete" |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_rep_incomplete", "customer.subscription.updated", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the retired subscription, incomplete, beside a replacement: %d; want 204", code) |
| + } |
| + if after, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || after.ID != second.ID { |
| + t.Fatalf("the replacement intent: %+v %v; want it untouched", after, err) |
| + } |
| +} |
diff --git a/internal/console/billingoperator.go b/internal/console/billingoperator.go| index ea310493e..31bd67cd7 100644 |
| --- a/internal/console/billingoperator.go |
| +++ b/internal/console/billingoperator.go |
| @@ -211,20 +211,39 @@ func (s *Server) handleAPIBillingReviewResolve(w http.ResponseWriter, r *http.Re |
| Reason string `json:"reason"` |
| Occurrences int64 `json:"occurrences"` |
| Note string `json:"note"` |
| + // Outcome, for a checkout refusal: "retire" (default) or "keep". |
| + Outcome string `json:"outcome"` |
| } |
| if !readJSON(w, r, &req) { |
| return |
| } |
| + if req.Outcome != "" && req.Outcome != "retire" && req.Outcome != "keep" { |
| + jsonError(w, http.StatusBadRequest, "outcome must be retire or keep") |
| + return |
| + } |
| acct, err := s.store.AccountBySqid(req.Account) |
| if err != nil || acct == nil { |
| jsonError(w, http.StatusNotFound, "no such billing review") |
| return |
| } |
| - err = s.store.resolveBillingReview(billingReviewResolution{AccountID: acct.ID, Subscription: req.Subscription, Reason: req.Reason, Occurrence: req.Occurrences, Actor: addr, Note: strings.TrimSpace(req.Note)}, time.Now()) |
| + err = s.store.resolveBillingReview(billingReviewResolution{AccountID: acct.ID, Subscription: req.Subscription, Reason: req.Reason, Occurrence: req.Occurrences, Actor: addr, Note: strings.TrimSpace(req.Note), Outcome: req.Outcome}, time.Now()) |
| switch { |
| case err == nil: |
| s.audit(acct, "", addr, "billing-review-resolve", req.Reason) |
| - writeJSON(w, http.StatusOK, map[string]any{"account": acct.Sqid, "subscription": req.Subscription, "reason": req.Reason, "resolved": true}) |
| + out := map[string]any{"account": acct.Sqid, "subscription": req.Subscription, "reason": req.Reason, "resolved": true} |
| + if req.Reason == billingCheckoutRefusedReason { |
| + // What the resolution did about the intent the review was keyed |
| + // by: retired it (the default), or kept it standing for the |
| + // corrected purchase to record. Nothing records under an open |
| + // review, so an intent is never gone at its resolution. |
| + retired, rerr := s.store.billingIntentRetired(acct.ID, req.Subscription) |
| + if rerr != nil { |
| + jsonError(w, http.StatusInternalServerError, "the retirement could not be read back") |
| + return |
| + } |
| + out["retired"] = retired |
| + } |
| + writeJSON(w, http.StatusOK, out) |
| case errors.Is(err, errBillingReviewInvalid): |
| jsonError(w, http.StatusBadRequest, "account, subscription, reason, occurrences and a note are required") |
| case errors.Is(err, errBillingReviewNotFound): |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index b4364f926..e40410895 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -798,15 +798,13 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| } else if !errors.Is(err, sql.ErrNoRows) { |
| return err |
| } |
| - // No intent stands and the subscription names one the operator |
| - // retired: settled for good, before any check whose retry could |
| - // never record it. |
| - if pending == nil { |
| - if retired, err := s.store.billingIntentRetired(acct.ID, sub.Metadata["checkout_intent"]); err != nil { |
| - return err |
| - } else if retired { |
| - return errBillingIdentity |
| - } |
| + // A subscription naming an intent the operator retired is settled |
| + // for good, before any check whose retry could never record it, |
| + // whether or not a later intent stands. |
| + if retired, err := s.store.billingIntentRetired(acct.ID, sub.Metadata["checkout_intent"]); err != nil { |
| + return err |
| + } else if retired { |
| + return errBillingIdentity |
| } |
| } |
| refuse := func() error { |
| @@ -849,10 +847,11 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| priceTerms, err := priceTermsFromMetadata(sub.Metadata) |
| if err != nil { |
| // A terms version this build does not know is a retry (a newer |
| - // build may know it) unless the subscription names a bound intent |
| - // this build made: that intent's own terms are known here, so a |
| - // newer build would refuse the mismatch too, and only the review |
| - // ends the retries. |
| + // build may know it) unless the subscription names the bound |
| + // standing intent: then only the review ends the retries (the |
| + // intent's terms are not compared here; an intent made by a newer |
| + // build before a rollback is reviewed rather than retried, as the |
| + // binder reviews its session, and that is consistent). |
| if errors.Is(err, plans.ErrUnknownTermsVersion) && !(pending != nil && pending.SessionID != "" && pending.ID == sub.Metadata["checkout_intent"]) { |
| return err |
| } |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index 2c57004c8..e0dc54a8b 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -755,7 +755,14 @@ func (s *Store) applyBillingStateDated(next billingState, expectedRevision int64 |
| } |
| } |
| if next.IntentID != "" { |
| - r, err := tx.Exec(`DELETE FROM billing_checkout_intents WHERE account_id=? AND id=?`, next.AccountID, next.IntentID) |
| + // The intent is consumed only if it stands and is not under an open |
| + // refusal review: a purchase corrected at the provider while its |
| + // review is open waits for the operator (who resolves the review |
| + // keeping the intent, or retires it); nothing records under a |
| + // review. |
| + r, err := tx.Exec(`DELETE FROM billing_checkout_intents WHERE account_id=? AND id=? |
| + AND NOT EXISTS (SELECT 1 FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=? AND resolved_at=0)`, |
| + next.AccountID, next.IntentID, next.AccountID, next.IntentID, billingCheckoutRefusedReason) |
| if err != nil { |
| return err |
| } |
| @@ -853,6 +860,11 @@ type billingReviewResolution struct { |
| // the operator bit), the console's existing operator identity. Note is a |
| // short technical reason code, never provider or customer data. |
| Actor, Note string |
| + // Outcome, for a checkout refusal: "retire" (the default, the money is |
| + // settled and the intent is retired) or "keep" (the purchase was |
| + // corrected at the provider; the intent stands and the next event |
| + // records it, or it expires as any other). |
| + Outcome string |
| } |
| const ( |
| @@ -948,7 +960,7 @@ func (s *Store) resolveBillingReview(r billingReviewResolution, now time.Time) e |
| // is recorded, and the row goes, so the next pass finds nothing to |
| // bind and the review does not come back. An intent that is not that |
| // one (a later purchase's) is left alone. |
| - if r.Reason == billingCheckoutRefusedReason { |
| + if r.Reason == billingCheckoutRefusedReason && r.Outcome != "keep" { |
| var sessionID, plan, band, currency, book string |
| err := tx.QueryRow(`SELECT session_id,plan,band,currency,price_book FROM billing_checkout_intents WHERE account_id=? AND id=?`, r.AccountID, r.Subscription).Scan(&sessionID, &plan, &band, ¤cy, &book) |
| if err != nil && !errors.Is(err, sql.ErrNoRows) { |