amadan
Multi-currency: the thirteenth review's findings
A first purchase's subscription edited to a plan, band or quantity the console does not sell is the intent-keyed review, not a raw error the provider retries for days; reconcile reads the bound session and expires it by its id before the review names it; a subscription naming a retired intent is refused and settled, never retried; the currency check has a test of its own. Comments and the design say what the code does. The thirteenth review's reports; dispositions corrected and "Review thirteen".
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8 files changed,
+406
−16
docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md+1 −1docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review13.md+141 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+18 −3docs/superpowers/specs/OPUS-REVIEW-multi-currency-review13.md+120 −0internal/console/billingcurrency_test.go+69 −5internal/console/billingpass.go+4 −2internal/console/billingservice.go+40 −5internal/console/billingstore.go+13 −0
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 c182d4055..f456f052a 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. |
| +- **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. |
| - **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-review13.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review13.md| new file mode 100644 |
| index 000000000..2526422a5 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review13.md |
| @@ -0,0 +1,141 @@ |
| +| Twelfth-review item | Status at `142293c5` | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **Opus P3-1 — contradictory bound session remains payable** | **Closed for successful expiry.** `ownBoundSession` expires the stored bound ID before filing, and leaves the intent standing (`internal/console/billingservice.go:294`). Failure and race behavior is qualified below. | **No.** `TestABoundSessionReadBackAsNobodysIsReviewed` requires one expiry, expired provider state and an unchanged standing intent (`internal/console/billingcurrency_test.go:3393`). It does not exercise expiry failure or completion racing expiry. | |
| +| **Opus P3-2 — edited `checkout_intent` suppresses the review** | **Closed.** The review attribution now requires the bound session ID, without trusting its changed intent metadata (`billingservice.go:298`). | **No.** The two-account cases require A’s referenced review and none for B (`billingcurrency_test.go:2818`, `:3274`). Restoring the removed clause suppresses A’s review. | |
| +| **Opus P3-3 — subscription price/currency refusal has no review** | **Closed for the reported price/currency checks; broader closure remains incomplete.** Those checks call `refuse()` (`billingservice.go:823`, `:826`), but earlier purchase-validation failures still bypass it. Finding 1 below. | **No when reverting the price-review fix. Yes when reverting only the currency-review fix.** Both new scenarios fail the price check first; Finding 2 explains the masking. | |
| +| **Opus P3-4 — reconcile and expected-binder review sites are unpinned** | **Closed.** Separate tests reach reconcile before any pass, and the binder after an unchanged first read (`billingcurrency_test.go:3622`, `:3644`). | **No.** Replacing either tested call site with plain ownership checking removes its required review. | |
| +| **Opus claim 1 — ownership comment is false** | **Corrected at `intentSession`; incomplete across the surrounding comments.** Its three exceptions are now explicit (`billingservice.go:241`). Other ownership absolutes remain stale, listed below. | Prose correction; no behavioral counterfactual. | |
| +| **Opus claim 2 — mutation count** | **Closed.** Review eleven now says three killed and two survived (`docs/superpowers/specs/DISPOSITIONS-multi-currency.md:242`). | Historical executions cannot be established from source. | |
| +| **Opus claim 3 — call-site coverage overstated** | **Closed by the new independent tests.** | **No**, as for P3-4. | |
| +| **Opus claim 4 — “expire by hand” lacks an operator door** | **Closed for automatic expiry succeeding.** The helper now performs the expiry itself. | **No**, as for P3-1. | |
| +| **Opus claim 5 — unused `acct` parameter** | **Closed.** Removed from `refuseSession` (`billingservice.go:338`). | Signature cleanup, not a behavioral fix. | |
| +| **Astra 1 — initial subscription validation can strand a paid purchase without review** | **Partly closed.** Price-only mismatches now receive the review; invalid plan/band/quantity and unknown terms still have early returns. | **No for the implemented price fix.** Existing tests still do not establish the broader claim. | |
| +| **Astra claims — count, subscription-review coverage, fake ID, ownership comment, site test, provider-call wording** | **Count and fake-ID coverage closed; ownership comment partly corrected; subscription coverage and site-test assurance still incomplete; provider-call wording remains false.** Details below. | The extended resolution test now **fails** if expiry always answers `cs_test_paid`: it actually expires `cs_test_second` (`billingcurrency_test.go:3022`). The site test still admits the counterexample in Finding 3. | |
| + |
| +These are source counterfactuals, not executed mutation tests. Platform references below are at **`142293c5`**, reviewed against **`12e70d95`**, including **`781bb1c2..142293c5`**. Site references are at **`251c4b1`**. Nothing was modified; no tests or network commands were run. |
| + |
| +The twelfth reports both grade their findings **P3**. Their substantive disagreement concerns expiry authority. **Opus’s conclusion was right for an already-bound session:** the stored ID was accepted for this intent before disclosure, and expiry addresses that ID, not another ID supplied by the contradictory response. Astra’s concern remains right for an unbound create/listing response. The new exception is appropriately restricted to the former. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P3 — Invalid first-subscription purchase fields still bypass the refusal review.** |
| + |
| + **Where:** `internal/console/billingservice.go:815`, `:820`; webhook handling at `internal/console/billinghandlers.go:303`, `:314`. |
| + |
| + **Scenario:** A GBP Personal checkout completes for one member. Before initial recording, someone changes the subscription item’s quantity to **2**, leaving its price, currency, metadata, customer and checkout-intent ID unchanged. |
| + |
| + `plans.PriceFor` rejects Personal quantity 2 through `Quote` (`internal/plans/pricebook.go:551`; `internal/plans/plans.go:303`). Reconcile returns that error directly at `billingservice.go:821`, before the intent mismatch reaches `refuse()`. An invalid plan or band follows the same route; unknown terms return even earlier. |
| + |
| + Both webhook orders leave the intent standing without a refusal review. These raw plan errors also bypass `acknowledgeBillingRejection`, producing **503 retries**, rather than an acknowledged identity rejection. The pass keeps encountering the same error. Nothing is incorrectly activated, but the paid unresolved purchase is absent from the operator’s review listing. |
| + |
| + **Smallest acceptable fix:** Once the standing bound intent is identified, route its subscription’s purchase-field validation failures through the intent-keyed review. Preserve the existing handling for subscriptions belonging to another intent and for already-recorded subscriptions. Add quantity-2 Personal, invalid-band and changed-terms cases with an otherwise unchanged completed session. |
| + |
| +2. **P3 — The new regression does not distinguish currency-review routing from a bare refusal.** |
| + |
| + **Where:** `internal/console/billingcurrency_test.go:3553`, `:3569`; the supposedly covered branch is `internal/console/billingservice.go:826`. |
| + |
| + **Scenario:** Revert only the currency mismatch’s `return refuse()` to `return errBillingIdentity`. |
| + |
| + The “another currency’s price” test changes **both** the price ID and the reported currencies. It therefore files through the preceding price-ID check at `billingservice.go:823`; it never reaches the currency check. The new test still passes. Existing independent currency checks begin with an already-recorded subscription and assert rejection and unchanged state, not an initial refusal review (`billingcurrency_test.go:1538`). |
| + |
| + **Smallest acceptable fix:** Keep the configured price ID, session and metadata unchanged. Change only the subscription currency, then only the item currency. For each initial-purchase case, require one referenced review and no recording. The price and currency mutations must be checked separately. |
| + |
| +3. **P3 — The site’s replacement amount test checks a permissive whitelist, not the amount’s correct context or provenance.** |
| + |
| + **Where:** Site `test/pricing-pricebook.test.mjs:169`, `:179`; static table generation at `copy-review/pricing/build.mjs:91`. |
| + |
| + **Scenario:** Replace `bandCell`’s static displayed amount with the literal **`$28`**, leaving its `data-currency-values` unchanged. Every static table cell then displays `$28`, including Community band D, whose USD price is `$2.10`. |
| + |
| + The new test accepts `$28` because it occurs elsewhere in the book. The table test checks the data attributes, not those static cell texts (`test/pricing-pricebook.test.mjs:76`). Thus this incorrect amount typed into the build script survives the claimed check. |
| + |
| + The whitelist also deliberately admits rounded forms of rates through `toFixed(2/3/4)`, and monthly divisions of annual prices. Those are weaker conditions than “this displayed amount is the exact exported amount for this slot.” |
| + |
| + **Smallest acceptable fix:** Compare every static money-bearing element with its own expected plan/band/currency or meter, alongside its attributes. Remove rounded-rate and unrelated-price fallbacks. If literal provenance itself must be enforced, use a source check or rebuild with changed book data; output equality alone cannot distinguish a correct literal from a derived value. |
| + |
| +**Results on the other requested boundaries** |
| + |
| +- **A — GBP purchase end to end.** `GET ?country=IN¤cy=gbp` quotes India’s band D in GBP: Personal **£7.70**, with **29.75 awake hours, 0.7 GB storage and 0.7 GB transfer**. The form carries country, currency, choice flag, member count and signature (`billingpage.go:265`; `templates/billing.html:50`). Its POST is compared against the signed effective currency before checkout (`billinghandlers.go:71`, `:102`); India additionally requires postal information. |
| + |
| + Checkout validates the GBP price’s amount and identity, then stores GBP/book on the intent (`billingservice.go:479`, `:511`, `:573`). `checkoutParams` preserves those fields, and `CreateCheckout` sends GBP with Adaptive Pricing disabled (`billingservice.go:221`; `internal/billingstripe/client.go:622`). |
| + |
| + Binding and initial reconciliation require ownership and purchase identity before recording (`billingservice.go:1070`, `:893`). Account and assignment writes retain GBP/book (`billingstore.go:599`, `:682`, `:725`). History reads both tables; monthly allowance selects the paid spans’ stored price terms; calculation produces a GBP version-2 snapshot (`billinginvoicestore.go:530`; `internal/plans/month.go:173`; `billingcalculation.go:174`). Invoice requests, recovery, verification and hashes use that snapshot’s currency (`billinginvoice.go:859`, `:886`, `:900`, `:940`, `:964`, `:1024`). Quantity processing selects and verifies the account’s stored price/currency (`billingquantity.go:1014`, `:1105`). **I found no alternate-currency calculation or historical repricing in this chain.** |
| + |
| +- **B/C/M/O/Q/U — Form, suggestions and signatures.** A changed country or effective currency requires a fresh quote. Forged agreeing fields cannot produce the server-keyed signature. Another tab’s cookies cannot alter an already-rendered form’s POST. A present currency must be one exact sold value: empty, padded, uppercase and repeated currency fields are refused on both doors, after their preceding authentication checks (`billinghandlers.go:176`). Other repeated fields are read consistently through `Get`; I found no parser disagreement that changes the charge. |
| + |
| + Without a currency field, a book console computes the country default; a legacy console computes EUR. The signed effective-currency comparison still applies. Changing or erasing `currency_chosen` cannot buy a different currency under the original signature. The quote door checks session, double-submitted token and membership, then constructs an encoded account-local redirect; it performs no billing mutation (`billinghandlers.go:137`, `:197`). |
| + |
| + The signing key derives from the server’s billing secret. The signature covers session token, account, book, country, band, currency, choice flag and members (`billingpage.go:446`). It can be reused for the **same tuple**, including an unchanged old tab; it cannot authorize another tuple, session, account, book or rotated token. Plan need not be signed because every selectable plan is quoted. Quantity follows signed membership and the plan minimum. Postal affects tax, excluded from these pretax figures. Membership is compared under the billing lock and rechecked transactionally under the membership lock (`billingservice.go:464`; `billingstore.go:348`). |
| + |
| + Present-empty parameters clear cookies; absent parameters fall back; invalid parameters change no cookie (`billingcurrency.go:43`). Consequently, an old cookie can reappear on a later parameter-absent GET after an **invalid** parameter, but not after successful clearing. Cookies populate the form; they do not choose at POST. Held currency overrides suggestions and is offered alone, including legacy EUR (`billingpage.go:301`). Checkout independently refuses another currency (`billingservice.go:560`). |
| + |
| +- **D/L/R/W/Y/AA/DD/FF/HH/KK — Ownership and expiry.** `ownSession` checks runtime mode, intent metadata, both account identifiers, customer once assigned and bound session ID (`billingservice.go:269`). Create, replay, listing, resume and binding use it before accepting a session as the purchase. Expected-intent binding checks the expected account before deriving one from the response. The pass’s two-account case cannot bind or record B while processing A. |
| + |
| + `ownBoundSession` is the explicit exception: a contradictory read of the **stored bound ID** can cause expiry and A’s review. Unowned create/listing responses remain untouched; the webhook binder with no expected intent does not attribute an ownership review from contradictory metadata. Filing checks that the exact intent still stands (`billingstore.go:970`). |
| + |
| + Expiry failure returns immediately and files **no new review on that call** (`billingservice.go:302`). If completion wins the race, the next completed-session read can file without another expiry. Repeated expiry failures while the session remains open delay the review and leave it payable; the intent remains standing. The new test does not cover these branches. |
| + |
| + A nil-error expiry answer is discarded completely, including its ID and status. Thus the review is **not proof of successful expiry**: the implementation relies on the expiry endpoint’s successful response semantics, and never uses that response to retire the intent. This distinction needs the test-mode checks below. |
| + |
| + Already-recorded webhook binding remains an explicit read-only fast path returning the recorded subscription (`billingservice.go:1057`). The success URL carries only `pending=1`; it does not itself record a purchase. |
| + |
| +- **EE/II/LL — Initial subscription versus binder.** The binder and reconcile use `intentSession` and the same intent-keyed review for completed-session purchase mismatches. Either ordering yields one occurrence while the intent stands. Finding 1 is a remaining “no review” case for subscription validation. |
| + |
| + A correct first purchase does not reach `refuse()`. Membership changes do not legitimately change its unrecorded provider quantity through the worker: that worker operates on recorded subscriptions (`billingquantity.go:918`, `:952`). Renewals use the recorded-subscription branch. An old canceled subscription either remains the recorded subscription or names an older intent; neither files a review against a legitimate newer intent. |
| + |
| + A subscription naming another intent receives a bare identity refusal: the webhook records rejection and answers **204**. With no standing intent, an otherwise valid unrecorded subscription reaches `sql.ErrNoRows` and **503**; earlier identity failures can instead receive **204**. The distinction remains intentional retry behavior, not retirement of another purchase. |
| + |
| +- **E/H/N/T — Barrier and migration.** The trigger matches the positive paid-span condition: active, non-Free, intersecting the month, ending at the earliest next assignment across both tables, paid-through instant or month end (`billingstore.go:208`; `internal/plans/month.go:146`). Same-second supersession gives the earlier revision zero duration. Free and inactive assignments terminate predecessors without contributing paid spans. |
| + |
| + There is one production writer of `billing_book_assignments`: it copies the subscription’s `period_end` as paid-through (`billingstore.go:725`). Proven renewal and quantity changes alter assignment `effective_at`, not that meaning (`:618`, `:655`). I found neither a valid history permitting an older version-1 freeze over a contributing book span nor one wrongly refusing this build’s valid legacy-only month. |
| + |
| + Replacement creation precedes old-trigger retirement. The retirement transaction checks the new trigger exists before dropping the old; a missing replacement fails without dropping it (`billingstore.go:797`). SQLite transactional DDL preserves that ordering. An interrupted migration can leave **both**, not neither; another successful start leaves only the new trigger. The existing successful-migration test does not independently establish the failure case. |
| + |
| + Startup adds the tables and legacy-default columns before use. The trigger does not make rollback generally safe: older binaries still cannot understand book accounts or new quantity outcomes. The documented rollback prohibition remains necessary. |
| + |
| +- **S/V/X/Z/BB/CC/GG/JJ — Retirement, references and history holds.** Resolution selects and deletes exactly the reviewed intent, records its session and purchase fields, and closes only its outstanding quantity events (`billingstore.go:922`, `:940`, `:943`, `:947`). A newer intent is untouched; late filing for a gone intent does nothing. An unbound refusal uses the review reference as its retirement session ID. The individual reader and operator API expose the reference, and an open older review can acquire it without another occurrence (`billingstore.go:784`, `:988`; `billingoperator.go:198`). |
| + |
| + `quantityOutcomesSQL` supplies all three admission checks. Changed processing-guard text is replaced at startup (`billingquantityaudit.go:20`, `:37`, `:61`). I found no production statement clearing an existing history gap; pre-history and clock-rollback holds survive startup. |
| + |
| + The resolution test now uses distinct sessions, supplies the retired session’s actual completion and asserts no subscription was recorded (`billingcurrency_test.go:3007`, `:3016`). It would fail if that completion activated billing state. Its new second-session expiry also pins the fake’s returned ID. |
| + |
| + Every provider-proof retirement uses `retireIntent`, including its transactional account-metadata check (`billingstore.go:441`, `:465`). Operator resolution and discarding an unready intent are separate authorities. Both legacy and book requests supply the account metadata now required in an expiry answer. |
| + |
| +- **F/G/J/K — Figures, validation and export.** Console inclusions divide `PriceFor`’s pooled allowance by its actual charged quantity; unavailable Personal uses the returned fallback quantity (`billingpage.go:395`, `:427`). Current figures have exact per-member shares at every supported quantity. Fractional displayed units, such as band-D Community’s **0.175 GB**, remain exact. The helper truncates an arbitrary nonrepresentable fraction, but the current terms’ divisibility checks exclude that case. Band A’s sentences are asserted verbatim (`billingcurrency_test.go:2628`). |
| + |
| + Book validation checks complete price matrices, checked rounding arithmetic, duplicate keys, country validity and legacy EUR equality (`internal/plans/pricebook.go:149`, `:231`). Configuration requires the selected catalog, complete optional catalogs and unique provider IDs (`cmd/console/billing.go:124`). Provisioning accepts the selected book’s 36 keys alongside other catalogs and applies the same own-only currency-option rule (`implementation/carloku-paid-launch/stripe-provision.sh:424`, `:434`). |
| + |
| + The export contains commercial definitions, not customer records or secrets. The site’s committed book and platform export are byte-identical. Rates, country options/defaults and scaled inclusions derive from it. The site rejects inexact per-member shares and figures; approved numeric strings are checked against the book. Draft builds receive the first-line marker, and the normal build/check requires exactly `PRICEBOOK_ALLOW_DRAFT=1` to admit it. |
| + |
| + Signup links always carry both parameters; currency is populated only after an explicit choice, country is the selected country (`src/site.js:74`). A locale-only GBP display can lead to a USD console default because the locale is deliberately not forwarded as a choice. With a selected country or explicit currency, I found no current interpretation mismatch. Finding 3 concerns the regression assurance, not a presently incorrect generated price. |
| + |
| +**Claims the code contradicts** |
| + |
| +- **“Every identity refusal” receives a review** remains too broad in dispositions `:263` and the reconcile comment at `billingservice.go:783`. Finding 1 gives a concrete counterexample. The new test also does not independently pin currency-review routing. |
| + |
| +- **The site checks that no amount is typed into the script** remains false (`test/pricing-pricebook.test.mjs:162`; dispositions `:265`). It now checks all currency symbols, but Finding 3 demonstrates the remaining gap. |
| + |
| +- **Several ownership absolutes were not updated.** `billingservice.go:267` says only sessions passing `ownSession` are expired; `:1016` says the two-account second read reviews nothing; `billingpass.go:187` similarly says it acts for nobody. The new helper intentionally expires or reviews A’s known bound session. Test comments at `billingcurrency_test.go:3036` and `:3360` still say it is not expired. |
| + |
| +- **The historical summary still overstates agreement.** Dispositions `:257` says both twelfth reviewers found every eleventh finding closed with distinguishing tests. Their pinned reports explicitly mark subscription-review coverage partial, and Astra identifies the then-unpinned fake-ID correction. |
| + |
| +- **“Before the provider is asked” remains at dispositions `:26`.** Checkout performs merchant/catalog/configuration reads before refusing the live subscription (`billingservice.go:490`, `:550`). It prevents another session creation. |
| + |
| +Review three’s default-as-choice mutation is correctly described as equivalent under the quote comparison (`DISPOSITIONS-multi-currency.md:102`). Review four now qualifies its provisioning-text checks, successful migration test and indistinguishable account-only delete mutation (`:108`, `:127`). Those source qualifications are reasonable; the historical scratch executions remain unverified. |
| + |
| +Fixtures still depend on real time for authentication, webhook freshness and relative session expiry (`billingservice_test.go:130`; `billingcurrency_test.go:1785`). I found no new calendar-date, ordering, temporary-root or shared-cookie-jar failure in this delta. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +1. **Catalog and purchase matrix:** Verify all 36 identities and expanded currency-option shapes. Complete purchases in USD/EUR/GBP, including IN/GBP and Community GBP bands B/D. Confirm actual unit amount, quantity, currency, tax and disabled Adaptive Pricing. |
| + |
| +2. **Independent mutations and event ordering:** Change session metadata independently of subscription metadata; change subscription price, currency and quantity separately. Deliver subscription/completion events in both orders and replay them. Valid purchases must record once; attributed mismatches must yield one referenced review and no activation. |
| + |
| +3. **Bound-session expiry and races:** Edit each mutable ownership field, then expire the stored bound ID. Confirm which fields Stripe permits editing, that expiry makes the held URL unusable, and what ID/status/metadata the answer carries. Force failure and completion racing expiry; establish the subsequent review and retry behavior. |
| + |
| +4. **Recovery and resolution:** Lose create responses, replay the original key and recover through listing. Resolve a refused intent, open a distinct replacement, then deliver the retired completion. The replacement must survive unchanged, with no old activation or reopened review. |
| + |
| +5. **Overages and legacy recovery:** Create, recover, finalize and pay invoices in each currency. Verify invoice/item/line currency, exact subtotal allocation, floors, tax, discounts and house waivers. Replay legacy EUR operations under their original parameter shapes. |
| + |
| +6. **Quantity and lifecycle:** Exercise increases, proration, renewal and the quantity fence in each currency. Verify price/currency acknowledgements and allowance dating. Establish customer-currency restrictions after cancellation and with outstanding invoice items. |
| + |
| +**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 ef8a24f90..5e3ca36e1 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -23,7 +23,7 @@ Reports: `OPUS-REVIEW-multi-currency-review1.md`, `CODEX-FINDINGS-multi-currency |
| | --- | --- | --- | --- | |
| | Astra P1, Opus P2-1: a session the provider created in another currency was refused on the click that created it, then found again by the replay or the listing and offered to the buyer. | Fixed. Every session the console binds, offers or records a purchase from passes one check against its intent (`intentSession`: this account's, this intent's, on the intent's price terms, in its currency with no Adaptive Pricing or conversion), however it was come by. A session that is this intent's and fails it is expired at the provider and the intent retired (`refuseSession`); the client names the session it refuses. A paid one the provider will not expire is left unbound for a person. | `billingservice.go`, `billingstore.go` `retireRefusedCheckout`, `billingstripe/client.go` | `TestCheckoutNeverOffersASessionInAnotherCurrency` (four cases: the click that created it, a failed expiry and a second click, the background pass, a bound session that reads back changed), `TestRecoveredSessionMustBeInTheIntentsCurrency` | |
| | Astra P1: an older build run against a migrated database reads a GBP assignment from the one table it knows, prices the month in EUR and freezes that. | Fixed. Assignments of a purchase priced from a book are kept in their own table (`billing_book_assignments`, with the currency and book required non-empty); the older build's own history query finds nothing for such a purchase, so it has no history to price. This build reads both tables as one. | `billingstore.go` `billingAllAssignments` | `TestBookPricedHistoryIsInvisibleToAnOlderBuild` (runs the older build's queries word for word) | |
| -| Opus P2-2: the design's refusal of a checkout in another currency while a subscription is live, or after one in a different currency, was not implemented. | Fixed. A live subscription refuses any checkout before the provider is asked; after it ends, a checkout in a currency other than the one bought in is refused, the country's default included. A change of currency is an operator's to make once Stripe's behaviour is known (test-mode list). | `billingservice.go` `startBillingCheckout` | `TestCheckoutAfterAPurchaseKeepsItsCurrency` | |
| +| Opus P2-2: the design's refusal of a checkout in another currency while a subscription is live, or after one in a different currency, was not implemented. | Fixed. A live subscription refuses any checkout (after the provider reads that find the subscription live, and before any session is created); after it ends, a checkout in a currency other than the one bought in is refused, the country's default included. A change of currency is an operator's to make once Stripe's behaviour is known (test-mode list). | `billingservice.go` `startBillingCheckout` | `TestCheckoutAfterAPurchaseKeepsItsCurrency` | |
| | Opus P2-3: a month whose paid spans differ in currency or book was held quietly, retried for ever, nobody told. | Fixed. `plans.ErrPriceTerms` is its own sentinel; the engine files `billing_invoice_price_terms:<month>` and freezes nothing. | `plans/month.go`, `billinginvoice.go` | `TestAllowanceForMonthKeepsOneCurrency`, `TestInvoiceEngineReviewsAMonthBoughtInTwoCurrencies` | |
| | Astra P2: configuration could not keep prices for a book no longer current, so publishing a new book would refuse subscriptions on the old one. | Fixed. The map holds the current book's catalog and may hold any other embedded book's and the legacy twelve, each whole or absent, no price under two keys. | `cmd/console/billing.go` | `TestBillingConfigPriceBookAndCatalog` | |
| | Astra P2, Opus P3-3: the invoice preflight checked the subscription's currency and not its item's; the quantity update's answer was confirmed without checking its price or currency. | Fixed: preflight requires exactly one item in the currency for a book purchase; the update's item must be on the account's own price in its currency. | `billinginvoice.go`, `billingquantity.go` | `TestSubscriptionAndItemCurrencyAreEachChecked`, `TestQuantityWorkerDoesNotConfirmAnUpdateOnAnotherPrice` | |
| @@ -254,14 +254,29 @@ Still open, as both reviewers list: everything that needs Stripe test mode (both |
| ## Review twelve (reviewed `781bb1c2`; Opus accepted with fixes, Astra accepted with fixes) |
| -Reports: `OPUS-REVIEW-multi-currency-review12.md`, `CODEX-FINDINGS-multi-currency-review12.md` (in the tree from this commit). Both found every eleventh-round finding closed, each with a test that fails without it, and nothing in a money path; both accept with fixes for the third round running. The two disagreed on one point, which the code settles below. Fixed, each with a test that fails without it; five mutants killed in a scratch copy (the bound session not expired; the intent-id clause restored; the price and currency checks back to a bare refusal; reconcile's and the binder's reads back to a plain ownership check): |
| +Reports: `OPUS-REVIEW-multi-currency-review12.md`, `CODEX-FINDINGS-multi-currency-review12.md` (in the tree from this commit). Both found the eleventh-round findings closed for the cases reported (Astra: the subscription review partial, and the fake-id fix unpinned, both taken up below), with tests that fail without their fixes, and nothing in a money path; both accept with fixes for the third round running. The two disagreed on one point, which the code settles below. Fixed, each with a test that fails without it; five mutants killed in a scratch copy (the bound session not expired; the intent-id clause restored; the price and currency checks back to a bare refusal, as one mutant, the price case the one the test reached [review thirteen: the currency case now has a test of its own, and the price-terms case is reached by the unknown-band case]; reconcile's and the binder's reads back to a plain ownership check): |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| -| Opus P3-1: the bound session read back as nobody's was left open at the provider, so after the operator resolved the review the buyer could still pay through the URL, with nothing recorded or reviewed. Astra (HH) held that leaving it untouched was right under the ownership rule. | Fixed as Opus asked, and the code decides it: the console already expires a session by its bound id when the buyer changes a purchase parameter, on the authority of having created it (the idempotency key names the intent), not on the authority of the read; the id is unique to the account and mode at the provider, so the expiry can make nothing but this intent's purchase unpayable; and the answer is used for nothing (`retireIntent` judges every retirement, and a contradictory answer retires nothing). So `ownBoundSession` expires an open bound session first, then files the review; the intent stands until the operator resolves it. A read that is not of the bound session still files nothing and expires nothing. | `billingservice.go` `ownBoundSession` | `TestABoundSessionReadBackAsNobodysIsReviewed` (one expiry by the bound id, the session expired at the provider, the intent standing and bound; then resolution and a new purchase); the two-account resume case in `TestASessionNamingTwoAccountsIsNobodys` | |
| +| Opus P3-1: the bound session read back as nobody's was left open at the provider, so after the operator resolved the review the buyer could still pay through the URL, with nothing recorded or reviewed. Astra (HH) held that leaving it untouched was right under the ownership rule. | Fixed as Opus asked, and the code decides it: the id expired is the one the console itself bound, written only after `intentSession` passed, so the read decides whether to expire and never supplies the id (the parameter-change expiry is the nearest precedent, though it follows a read already judged the intent's own); the id is unique to the account and mode at the provider, so the expiry can make nothing but this intent's purchase unpayable; and the answer is used for nothing (`retireIntent` judges every retirement, and a contradictory answer retires nothing). So `ownBoundSession` expires an open bound session first, then files the review; the intent stands until the operator resolves it. A read that is not of the bound session still files nothing and expires nothing. | `billingservice.go` `ownBoundSession` | `TestABoundSessionReadBackAsNobodysIsReviewed` (one expiry by the bound id, the session expired at the provider, the intent standing and bound; then resolution and a new purchase); the two-account resume case in `TestASessionNamingTwoAccountsIsNobodys` | |
| | Opus P3-2: a bound session whose `checkout_intent` metadata was edited still wedged the intent in silence (the clause requiring the session's intent metadata to match). | Fixed: the bound id alone identifies the console's own session; the clause is gone. The two-account second-read cases now expect A's one review naming A's session, and none for B. | `billingservice.go` `ownBoundSession` | `TestBindersSecondReadIsJudgedAgainstTheIntent`, `TestOwnershipIsEnforcedOnEveryPath` ("another account's everything") | |
| | Opus P3-3, Astra 1 (P3): a subscription naming the bound intent whose price or currency was changed at the provider was a bare refusal (those checks returned before the intent was read). | Fixed: for a first purchase the standing intent is read before the subscription is judged, and every identity refusal of a subscription that names that bound intent (its price terms, its price, its currency, its plan, band, terms or quantity) files the intent-keyed review with the bound session as reference; a subscription naming another intent, or with no intent standing, is refused as before. | `billingservice.go` `reconcileBillingSubscription` (`refuse`) | `TestASubscriptionRefusedForItsPriceAloneIsReviewed` (another band's price, another currency's price; subscription first, completion first, the pass) | |
| | Opus P3-4: reconcile's and the expected-intent binder's uses of `ownBoundSession` were not told from a plain ownership check. | Fixed: a subscription event before any pass, with the session's customer alone changed, files through reconcile; a pass whose binder reads the session a second time and finds it another customer's files through the binder. | tests | `TestEachReaderOfTheBoundSessionFilesTheReview` | |
| | Astra claims: the eleventh summary's mutant count; the subscription-review row overstated; the fake-id regression unpinned; the ownership comment; the site's euro test vacuous; a review-two sentence about when a live subscription is refused. Opus claims: the ownership comment; the `ownBoundSession` comment ("by hand"); `refuseSession`'s unused parameter; the row claim about reconcile and the binder. | Corrected in place: three killed, two surviving; the rows above; the resolution test expires and retires `cs_test_second`; the `intentSession` comment names its three exceptions; `refuseSession` lost the parameter; the site test (site `251c4b1`) checks every amount on the page in all three currencies against the book or an approved string. | this file, `billingservice.go`, site `test/pricing-pricebook.test.mjs` | | |
| Still open, as both reviewers list: everything that needs Stripe test mode (both lists, with session metadata edits, resolve-then-pay, a price swapped between completion and recording, and the expire response's fields added this round), the two unreviewed system strings, the operator's door for a quantity-history gap, a listing of rejected webhook events for the operator. The thirteenth review reads this commit. |
| + |
| +## Review thirteen (reviewed `142293c5`; Opus accepted with fixes, Astra accepted with fixes) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review13.md`, `CODEX-FINDINGS-multi-currency-review13.md` (in the tree from this commit). Both found the twelfth-round findings closed for the cases reported, each with a test that fails without its fix, and nothing in a money path; both accept with fixes for the fourth round running, and both side with expiring the bound session (the id the console itself bound; the read decides whether, never which). Fixed, each with a test that fails without it; four mutants killed in a scratch copy (`PriceFor`'s errors returned raw; the hand-made subscription's session not expired; the retired intent's events left a retry; the currency check back to a bare refusal): |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P3-1, Astra 1 (P3): a first purchase's subscription edited to a plan, band or quantity the console does not sell returned `PriceFor`'s error raw: no review, and 503 retries for days. | Fixed: `ErrInvalidQuantity`, `ErrUnknownPlan` and `ErrUnknownBand` go through `refuse()`. A terms version this build does not know stays a raw error on purpose: a newer build may know it (a deploy in progress), and the console cannot tell that from an edit at the provider. | `billingservice.go` `reconcileBillingSubscription` | `TestASubscriptionRefusedForItsPriceAloneIsReviewed` ("a quantity the plan does not sell", "a band the console does not know"; subscription first, completion first, the pass) | |
| +| Opus P3-2: reconcile's review named the bound session without reading it, so a session still open (a subscription made by hand at the provider) survived the resolution and could be paid. | Fixed: `refuse()` reads the bound session and, if it is open, expires it by its bound id before filing, as `ownBoundSession` does; the answer is used for nothing. | `billingservice.go` `refuse` | `TestAHandMadeSubscriptionExpiresTheOpenBoundSession` | |
| +| Opus P3-3: after a resolution, every event of the refused subscription was answered 503 for the life of the subscription (no intent stands). | Fixed: a subscription naming an intent in `billing_retired_intents` is refused and settled (one rejected-events row, 204); with no intent standing and no retirement, the retry stands. | `billingservice.go`, `billingstore.go` `billingIntentRetired` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` (two subscription events after the resolution answer 204) | |
| +| Astra 2 (P3): the currency branch of `refuse()` was not told from a bare refusal (the test changed the price too). | Fixed: a case with the intent's own price and the subscription's money in another currency reaches the currency check alone. | tests | the same price test ("the right price in another currency") | |
| +| Astra 3 (P3): the site test admitted a literal typed in place of a slot's value when it equalled some other book amount, and matched strings by substring. | Fixed (site `5be6b41`): every element carrying per-currency or per-band values must show, as static text, exactly its own USD (band A) value; amounts match an approved string as a whole token; the rounded-rate and monthly-share fallbacks are gone. | site `test/pricing-pricebook.test.mjs` | | |
| +| Claims (both): `ownSession`'s "only such a session is ever expired"; the pass's "acted on for nobody"; the design document's checkout bullet; dispositions line 26; the twelfth summary's mutant sentence and agreement sentence; the precedent for the expiry's authority; the reconcile comment's "anything". | Corrected in place (the design bullet now states the bound-session expiry and review and reconcile's review; the comment names the terms-version exception). | this file, `billingservice.go`, `billingpass.go`, the design | | |
| + |
| +Open, as both list: everything that needs Stripe test mode (both lists, with the expiry after a metadata edit, the idempotency key across outcomes, an out-of-range edit before recording, a hand-made subscription, the events after a resolution, and a price or currency swap on a locked customer added this round), the two unreviewed system strings, the operator's door for a quantity-history gap, a listing of rejected webhook events; expiry failure and the completion race are stated in `ownBoundSession` (nothing is filed on that read; the next read files). The fourteenth review reads this commit. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review13.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review13.md| new file mode 100644 |
| index 000000000..06ad2c880 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review13.md |
| @@ -0,0 +1,120 @@ |
| +# Thirteenth review of `feat/carloku-multi-currency-20261001` at `142293c5` (base `12e70d95`) |
| + |
| +**Verdict: accept with fixes.** All of the twelfth round's findings are closed for the cases they reported. Each fix has a test that fails without it, apart from one branch of one fix (below). Nothing in this round changes how money is computed, sent to Stripe or stored. Three P3 defects remain, plus some claims to correct. The most important: a subscription whose quantity, plan or band was edited at Stripe to a value the console does not sell still fails in silence. Stripe retries it forever and no operator review is filed. |
| + |
| +## Scope |
| + |
| +- **The round's delta (`781bb1c2..142293c5`):** |
| + - `billingservice.go`: `ownBoundSession` now expires, the `checkout_intent` clause is dropped, reconcile gained `refuse()`, and `refuseSession` lost its `acct` parameter. |
| + - `billingpass.go`: call signatures only. |
| + - Tests and the dispositions. |
| + - The site test at `251c4b1`. |
| +- **Unchanged since earlier rounds:** `internal/plans`, `cmd`, `billingstripe`, `billingpage.go`, `billinghandlers.go`, `billingcurrency.go`, the trigger code, the quantity code and `billingstore.go`. Sections A–C, E–J, M–Q, T–X and Z–DD stand as rounds ten to twelve traced them. I re-read only where the delta reaches: `retireIntent`, `fileCheckoutRefusedReview`, `resolveBillingReview`, the pass, the binder and the webhook handler. |
| +- I read source with `git show` only, modified nothing and ran nothing. My scratch copies are deleted. |
| + |
| +## The twelfth review's findings |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1**: a bound session read back as nobody's was left payable after resolution | **Closed.** `ownBoundSession` (`billingservice.go:294-312`) expires an open bound session by its bound id, then files the review. The intent stands until resolution. | **No.** `TestABoundSessionReadBackAsNobodysIsReviewed` (`billingcurrency_test.go:3367`) now requires one expiry and the session `expired`. So does the two-account resume case in `TestASessionNamingTwoAccountsIsNobodys`. | |
| +| **Opus P3-2**: an edited `checkout_intent` on the bound session wedged the intent in silence | **Closed.** The clause is gone. The bound id alone decides (`:298`). | **No.** "Another account's everything" in `TestOwnershipIsEnforcedOnEveryPath`, and `TestBindersSecondReadIsJudgedAgainstTheIntent`, now expect A's one review naming A's session. With the clause present, the read names B's intent and nothing is filed. | |
| +| **Opus P3-3 / Astra 1**: a price or currency swap on a subscription naming the bound intent was a bare refusal | **Closed for price.** `refuse()` (`:798`) runs at `:818`, `:824`, `:827` and `:861`. **Two gaps:** `PriceFor`'s errors at `:820` are still returned raw (P3-1 below), and the currency branch `:827` has no test of its own (claim 4). | **No for price.** `TestASubscriptionRefusedForItsPriceAloneIsReviewed` (`:3552`) is a bare refusal without the fix, in all three orders. **Yes for the currency branch:** the "another currency's price" case (`:3555`) is caught by the price check at `:824` first. | |
| +| **Opus P3-4**: reconcile's and the binder's calls not told apart from plain `ownSession` | **Closed.** | **No.** In `TestEachReaderOfTheBoundSessionFilesTheReview` (`:3612`), the reconcile case changes only `sessionCustomer`, so reconcile is the first reader. In the binder case, the pass's own read passes and the binder's second read fails. Nothing else files in either case. | |
| +| **Claims** (both reviewers) | **Mostly closed.** Two remain: the design document is unchanged, and the dispositions' line 26 still says "before the provider is asked" (claims 2 and 3). | n/a | |
| + |
| +**The disagreement (HH/KK: expire or leave open).** The code sides with expiring, and that is the right reading: |
| + |
| +- The id expired is `intent.SessionID`. It is written only by `saveBillingCheckout` and the binder's `UPDATE`, and both run after `intentSession` has passed. The read only decides *whether* to expire. It never supplies the id. |
| +- The answer to the expiry is discarded. A later retirement still needs `retireIntent`'s full proof, and a contradicting answer cannot give that. |
| + |
| +Astra's worry was "a separate authority rule for contradictory responses". It does not hold, because the authority comes from the binding, not from the response. One caveat: the disposition's precedent is not exact. The expiry on a parameter change (`:617`) runs only after the session was judged the intent's own (claim 5). |
| + |
| +## Findings |
| + |
| +### P3-1. A first-purchase subscription whose quantity, plan or band was edited to a value the console does not sell gets no review, and Stripe retries it forever |
| + |
| +- **Where:** `billingservice.go:820`. `plans.PriceFor` errors are returned raw: `ErrInvalidQuantity`, `ErrUnknownPlan` and `ErrUnknownBand` from `Quote`. |
| +- **Scenario:** |
| + 1. A GBP Personal purchase completes on `cs_X`. |
| + 2. Before it is recorded, someone at Stripe sets the subscription's quantity to 2 (outside Personal's range), or edits `metadata.band` to an unknown value. |
| + 3. The subscription event: reconcile reaches `:820` and returns the raw error. The handler answers 503, so Stripe retries for about three days. |
| + 4. The completion event: the binder binds the untouched session, then reconcile fails the same way, so 503 again. |
| + 5. Every pass fails the same way. |
| + 6. No review is filed, the intent stands, and the account can neither buy nor be deleted. |
| +- **Contradicted claim:** the dispositions' row for this finding says every identity refusal of a subscription naming the bound intent files the review, "(its … quantity)". |
| +- **Smallest fix:** at `:820`, return `refuse()` for `ErrInvalidQuantity`, `ErrUnknownPlan` and `ErrUnknownBand`. Keep `ErrUnknownTermsVersion` raw, as `:815` does. |
| +- **Test:** set `subscriptionQuantity` out of range with everything else unchanged. Expect one review in both event orders and through the pass. |
| + |
| +### P3-2. Reconcile's review names the bound session without reading it, so a session still open survives the resolution |
| + |
| +- **Where:** `refuse()`, `billingservice.go:798-805`. The resolution retires the intent whatever the session's state (`billingstore.go:922-948`). |
| +- **Scenario:** |
| + 1. Intent I is bound to `cs_X`, still open. |
| + 2. Someone at Stripe creates a subscription by hand, carrying I's metadata at another price. |
| + 3. Reconcile's `refuse()` files the review naming `cs_X` without reading it. |
| + 4. The operator resolves, and I is retired. |
| + 5. The buyer pays through `cs_X`'s URL. The webhook binder refuses it, because no intent stands. The subscription event answers 503 (`:845`) with nothing standing, or a bare refusal if a new intent stands. |
| + 6. A charge with no record. |
| + |
| + This is the twelfth round's P3-1 shape, through the one review path that does not read the session. It needs someone at the provider, which keeps it at P3. |
| +- **Smallest fix:** in `refuse()`, read `pending.SessionID`. If it is open, expire it by its bound id, exactly as `ownBoundSession` does, and then file. |
| + |
| +### P3-3. Every event of a refused subscription answers 503 after resolution, for the life of that subscription |
| + |
| +- **Where:** `billingservice.go:842-845`. With no intent standing it returns `sql.ErrNoRows`, which the handler maps to 503 (`billinghandlers.go:300-308`). |
| +- **Scenario:** |
| + 1. A session completes in the wrong currency. The review is filed, and the operator refunds, cancels and resolves. |
| + 2. The intent is retired, with a row in `billing_retired_intents`. |
| + 3. From then on, every event on that subscription is answered 503 and retried by Stripe for days: `updated`, `deleted`, `invoice.paid` and replays alike. |
| + 4. No retry can succeed. Once retired, the intent never stands again. |
| +- **Smallest fix:** when no intent stands and `sub.Metadata["checkout_intent"]` is in `billing_retired_intents` for the account, return `errBillingIdentity`. That is settled: one rejected-events row, then 204. |
| + |
| +## Added sections |
| + |
| +- **KK. Can the expiry act on another session; what if it fails; can the expiry and review disagree?** |
| + - **Another session:** no. See the disagreement above. The idempotency key `carloku-expire-<intent>` is shared with `refuseSession` and the parameter-change expiry. All three name the same bound id within one intent's life, so a replay returns the same answer. |
| + - **The expiry fails, or the session completes under the read:** `ownBoundSession` returns the error, and nothing is filed on that read. The next read finds the session `complete` (no expiry is attempted) and files. If Stripe stores a 400 against the key, that cannot block the later filing, because a completed session is never expired again. |
| + - **Expiry and review disagreeing:** possible only in two benign orders: |
| + - The intent was retired between the pass's snapshot and the filing (the pass holds no lock). The expiry then hits the retired intent's own session, and `fileCheckoutRefusedReview` files nothing. |
| + - The filing fails after the expiry. The next read finds the session `expired` and files. |
| + - **Not a defect:** the provider's metadata could be edited back after the expiry. The pass's `expired` branch would then retire the intent while its review is still open. The review then resolves with no retirement row. Harmless. |
| +- **LL. Can a legitimate subscription file `refuse()`; what does the webhook answer?** |
| + - No legitimate subscription can file it: |
| + - A renewal, or the recorded subscription's late event, has `state.Subscription == id`, so `pending` is nil. |
| + - An older intent's subscription names an intent other than the standing one. |
| + - Intents are deleted on record (`billingstore.go:746`). |
| + - One gap: if an operator drops a catalog from configuration while an intent on it stands, the legitimate purchase files a review. That is the right outcome. |
| + - The bare refusal for a subscription naming another intent is right. |
| + - The webhook answers: |
| + |
| + | Case | Answer | |
| + |---|---| |
| + | `refuse()`, or a bare refusal | 204, with a rejected-events row | |
| + | No intent standing (P3-3) | 503 | |
| + | A raw `PriceFor` error (P3-1) | 503 | |
| +- **EE (re-checked).** One review across reconcile and the binder, because the review is filed once while open. "None" remains only for P3-1. |
| +- **MM. Does the site test at `251c4b1` now fail for an amount typed in the script?** For an invented or stale amount, yes, in every currency. It checks membership, not provenance: |
| + - A typed amount equal to any book value of that currency passes, whatever its plan or band, as does one-twelfth of one (the `share` clause). Placement is caught elsewhere only for the plan prices, the band table, the rates and the floor. |
| + - `inStrings` uses substring `includes`. A typed `€2` passes on `€24/y`, and `€1` passes on `Minimum €1 bill`. |
| + - To tighten it, match each candidate as a whole token and assert it against the specific slot's export value. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **`billingservice.go:267`** (`ownSession`): "Only such a session is ever expired … on the intent's behalf." `ownBoundSession` (`:301`) now expires exactly the bound sessions that fail it. The comment at `:240-247` states the exception, but this sentence does not. |
| +2. **The design document is unchanged** (line 152). It says nothing about a bound session being expired and reviewed. It still calls a subscription on the wrong price "an identity mismatch", where for a first purchase naming the bound intent it is now a review. This is the twelfth round's Opus claim 4, which the dispositions mark as corrected. |
| +3. **Dispositions line 26** still says "refuses any checkout before the provider is asked". The review-twelve row says it was corrected in place. The correction exists only as the review-two row at `:66`. |
| +4. **Dispositions, review twelve:** "five mutants killed (… the price and currency checks back to a bare refusal)". This holds only as a single mutant. Reverting the currency `refuse()` at `:827` alone survives, because no test reaches it with a matching price. The price-terms `refuse()` at `:818` is not tested either. |
| +5. **The review-twelve row for Opus P3-1** cites the parameter-change expiry as precedent for "the authority of having created it". That expiry follows a read already judged the intent's own (`:603-617`). The conclusion holds; the precedent is not exact. |
| + |
| +## Verify in Stripe test mode before enabling |
| + |
| +All earlier lists stand. Add, or sharpen: |
| + |
| +1. **Expire after a metadata edit.** Edit an open session's `account_sqid`, customer or client reference. Confirm the console's expiry of the bound id succeeds and the URL stops accepting payment. |
| +2. **The idempotency key across outcomes.** Expire with `carloku-expire-<intent>` while the session completes concurrently. Check whether Stripe stores the 400 against the key, and that the later read files the review. |
| +3. **Quantity or metadata edited out of range before recording** (P3-1). Today this shows 503s with no review. |
| +4. **A subscription created by hand naming an intent whose session is open, then resolve and pay** (P3-2). |
| +5. **Events of a refused subscription after resolution** (P3-3). Shows how many 503 retries arrive, and whether Stripe warns about or disables the endpoint. |
| +6. **A price swap and a currency swap on a live subscription for a customer locked to a currency.** Shows whether Stripe refuses a cross-currency swap (this decides whether `:827` is reachable at all). |
| + |
| +**Verdict: accept with fixes** (P3-1 to P3-3 and the five claims). |
diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go| index d6cba499d..a299659c8 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -2931,6 +2931,14 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| if _, err := fx.st.billingCheckoutIntent(acct.ID); err == nil { |
| t.Fatal("the refused intent still stands after its review was resolved") |
| } |
| + // Every later event of the refused subscription is settled (refused, |
| + // acknowledged), never a retry: no intent stands and the one it names |
| + // was retired. (Review thirteen, Opus P3-3.) |
| + for i, typ := range []string{"customer.subscription.updated", "customer.subscription.deleted"} { |
| + if code := f.webhookPost(t, fx, fmt.Sprintf("evt_after_resolution_%d", i), typ, "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("%s after the resolution: %d; want it settled with 204, not retried", typ, code) |
| + } |
| + } |
| 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) |
| @@ -3035,8 +3043,9 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| // A session that names two accounts (one in its client reference, another |
| // in its metadata) is nobody's: not offered again, not bound from a |
| -// listing, and not expired either, since the console cannot say whose it |
| -// is. (Review ten, Astra 2: the binder refused such a session and the |
| +// listing; the bound one is expired by the id the console itself bound |
| +// and reviewed, the listed one left alone, since the console cannot say |
| +// whose it is. (Review ten, Astra 2: the binder refused such a session and the |
| // resume and listing paths did not.) |
| func TestASessionNamingTwoAccountsIsNobodys(t *testing.T) { |
| click := func(fx *consoleFixture, f *fakeBillingStripe, acct *Account, after time.Duration) (string, error) { |
| @@ -3551,8 +3560,11 @@ func TestAnExpiryAnsweredForAnotherAccountRetiresNothing(t *testing.T) { |
| // the intent was read.) |
| func TestASubscriptionRefusedForItsPriceAloneIsReviewed(t *testing.T) { |
| for name, price := range map[string]string{ |
| - "another band's price": currencyPriceID(plans.PlanPersonal, plans.BandD, plans.GBP), |
| - "another currency's price": currencyPriceID(plans.PlanPersonal, plans.BandA, plans.USD), |
| + "another band's price": currencyPriceID(plans.PlanPersonal, plans.BandD, plans.GBP), |
| + "another currency's price": currencyPriceID(plans.PlanPersonal, plans.BandA, plans.USD), |
| + "the right price in another currency": "", |
| + "a quantity the plan does not sell": "", |
| + "a band the console does not know": "", |
| } { |
| for _, order := range []string{"subscription first", "completion first", "the pass"} { |
| t.Run(name+", "+order, func(t *testing.T) { |
| @@ -3567,8 +3579,24 @@ func TestASubscriptionRefusedForItsPriceAloneIsReviewed(t *testing.T) { |
| } |
| f.mu.Lock() |
| f.sessionStatus, f.subscriptionPrice = "complete", price |
| - if strings.Contains(name, "currency") { |
| + switch name { |
| + case "another currency's price": |
| f.subscriptionCurrency, f.itemCurrency = "usd", "usd" |
| + case "the right price in another currency": |
| + // The price is the intent's; the subscription's money |
| + // is not (reaches the currency check alone). |
| + f.subscriptionPrice, f.subscriptionCurrency, f.itemCurrency = intent.Price, "usd", "usd" |
| + case "a quantity the plan does not sell": |
| + f.subscriptionPrice, f.subscriptionQuantity = intent.Price, 2 |
| + case "a band the console does not know": |
| + // The subscription's own metadata edited to a band that |
| + // is not sold; the session's metadata untouched. |
| + sessionMeta := map[string]string{} |
| + for k, v := range f.meta { |
| + sessionMeta[k] = v |
| + } |
| + f.sessionMeta, f.subscriptionPrice = sessionMeta, intent.Price |
| + f.meta["band"] = "Z" |
| } |
| f.mu.Unlock() |
| post := func(id, typ, object string) { |
| @@ -3663,3 +3691,39 @@ func TestEachReaderOfTheBoundSessionFilesTheReview(t *testing.T) { |
| } |
| }) |
| } |
| + |
| +// A subscription made by hand at the provider, naming a bound intent whose |
| +// session is still open and not matching its purchase: reconcile expires |
| +// the bound session by its id before the review names it, so nothing can |
| +// be paid through it after the operator resolves. (Review thirteen, Opus |
| +// P3-2.) |
| +func TestAHandMadeSubscriptionExpiresTheOpenBoundSession(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, err := fx.st.billingCheckoutIntent(acct.ID) |
| + if err != nil || intent.SessionID == "" { |
| + t.Fatalf("fixture: intent %+v %v", intent, err) |
| + } |
| + f.mu.Lock() |
| + // The session stays open; a subscription exists for the intent at |
| + // another band's price. |
| + f.subscriptionPrice = currencyPriceID(plans.PlanPersonal, plans.BandD, plans.GBP) |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_hand_made", "customer.subscription.created", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the subscription event: %d", code) |
| + } |
| + if f.expireCalls != 1 || f.sessionStatus != "expired" { |
| + t.Fatalf("%d expiries, session %s; want the open bound session expired before the review named it", f.expireCalls, f.sessionStatus) |
| + } |
| + var occurrences int64 |
| + var reference string |
| + if err := fx.st.db.QueryRow(`SELECT occurrences, reference FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=? AND resolved_at=0`, acct.ID, intent.ID, billingCheckoutRefusedReason).Scan(&occurrences, &reference); err != nil || occurrences != 1 || reference != intent.SessionID { |
| + t.Fatalf("review: %d occurrences, reference %q, %v; want one review naming the session", occurrences, reference, err) |
| + } |
| + if standing, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || standing.ID != intent.ID || standing.SessionID != intent.SessionID { |
| + t.Fatalf("intent %+v %v; want it standing, bound as it was (the expiry's answer retires nothing)", standing, err) |
| + } |
| +} |
diff --git a/internal/console/billingpass.go b/internal/console/billingpass.go| index 10eea8830..9f0273b8b 100644 |
| --- a/internal/console/billingpass.go |
| +++ b/internal/console/billingpass.go |
| @@ -186,8 +186,10 @@ func (s *Server) reconcilePendingIntent(ctx context.Context, intent billingInten |
| } |
| // The intent's own session, judged against the intent this pass is |
| // working on, before anything is dispatched on it: a response |
| - // carrying the right id and another account's everything else is |
| - // acted on for nobody. |
| + // carrying the right id and another account's everything else |
| + // records and binds nothing for anybody; being the session this |
| + // intent is bound to, it is expired by that id and reviewed |
| + // (ownBoundSession). |
| owner, err := s.store.AccountByID(intent.AccountID) |
| if err != nil || owner == nil { |
| return err |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index a00e4936b..2f9824d39 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -264,8 +264,10 @@ func (b *billingRuntime) intentSession(session *billingstripe.Session, intent bi |
| // session that names two accounts is nobody's), in this mode, carrying |
| // the intent's id, for the intent's own customer (once the intent has |
| // one), and, once the intent is bound, the very session it is bound to. |
| -// Only such a session is ever expired, offered, bound or recorded on the |
| -// intent's behalf. |
| +// Only such a session is ever offered, bound or recorded on the intent's |
| +// behalf, or expired on proof; the one session expired without passing |
| +// here is the intent's own bound session read back as nobody's |
| +// (ownBoundSession, by the id the console itself bound). |
| func (b *billingRuntime) ownSession(session *billingstripe.Session, intent billingIntent, acct *Account) bool { |
| if session == nil || session.ID == "" || session.LiveMode != b.cfg.LiveMode || session.Metadata["checkout_intent"] != intent.ID || session.ClientReferenceID != acct.Sqid || session.Metadata["account_sqid"] != acct.Sqid { |
| return false |
| @@ -784,7 +786,9 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| // the standing intent is read before the subscription is judged: a |
| // subscription that names that intent and is bound to a session, and |
| // is then refused for anything (its price, its currency, its plan or |
| - // band, its quantity), is the intent-keyed review with that session as |
| + // band, its quantity; not a terms version this build does not know, |
| + // which a newer build may, and so stays a retry), is the intent-keyed |
| + // review with that session as |
| // reference, so a change made to it at the provider between payment |
| // and recording reaches a person rather than a rejected-events row. |
| var pending *billingIntent |
| @@ -797,6 +801,21 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| } |
| refuse := func() error { |
| if pending != nil && pending.SessionID != "" && pending.ID == sub.Metadata["checkout_intent"] { |
| + // The bound session is read before the review names it: one |
| + // still open (a subscription made by hand at the provider |
| + // while the buyer's session waits) is expired by its bound id |
| + // first, as ownBoundSession does and for the same reason, so |
| + // nothing can be paid through it after the operator resolves |
| + // the review; the answer is used for nothing. |
| + session, err := b.client.GetCheckout(ctx, pending.SessionID) |
| + if err != nil { |
| + return err |
| + } |
| + if session.ID == pending.SessionID && session.Status == "open" { |
| + if _, err := b.client.ExpireCheckout(ctx, pending.SessionID, "carloku-expire-"+pending.ID); err != nil { |
| + return err |
| + } |
| + } |
| if err := s.store.fileCheckoutRefusedReview(*pending, pending.SessionID, now); err != nil { |
| return err |
| } |
| @@ -818,6 +837,14 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| return refuse() |
| } |
| if _, err := plans.PriceFor(priceTerms, plan, band, sub.Items[0].Quantity); err != nil { |
| + // A plan, band or quantity the console does not sell is not a |
| + // purchase, and for a first purchase naming the bound intent it is |
| + // the same review as any other refusal (a quantity edited out of |
| + // range at the provider, say), never an error the provider is |
| + // told to retry for days. |
| + if errors.Is(err, plans.ErrInvalidQuantity) || errors.Is(err, plans.ErrUnknownPlan) || errors.Is(err, plans.ErrUnknownBand) { |
| + return refuse() |
| + } |
| return err |
| } |
| if plan == plans.PlanFree || sub.Items[0].PriceID == "" || sub.Items[0].PriceID != b.cfg.Prices[billingPriceKey(priceTerms, plan, band)] { |
| @@ -840,8 +867,16 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| return errBillingIdentity |
| } |
| if pending == nil { |
| - // No intent stands: nothing this subscription could be the |
| - // purchase of. Not settled (the provider tries again). |
| + // No intent stands. A subscription naming an intent the |
| + // operator retired (a refusal resolved) is settled for good: |
| + // refused, once, and every later event of it the same, never a |
| + // retry. Anything else is not settled (the provider tries |
| + // again). |
| + if retired, err := s.store.billingIntentRetired(acct.ID, sub.Metadata["checkout_intent"]); err != nil { |
| + return err |
| + } else if retired { |
| + return errBillingIdentity |
| + } |
| return sql.ErrNoRows |
| } |
| intent := *pending |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index 3044deaca..47d7f8910 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -779,6 +779,19 @@ type billingReview struct { |
| Reference string |
| } |
| +// billingIntentRetired reports whether an intent of this account was |
| +// retired by an operator's resolution (billing_retired_intents). |
| +func (s *Store) billingIntentRetired(accountID int64, intentID string) (bool, error) { |
| + if intentID == "" { |
| + return false, nil |
| + } |
| + var n int |
| + if err := s.db.QueryRow(`SELECT count(*) FROM billing_retired_intents WHERE account_id=? AND intent_id=?`, accountID, intentID).Scan(&n); err != nil { |
| + return false, err |
| + } |
| + return n > 0, nil |
| +} |
| + |
| // billingReview is the operator read a resolution must quote: the |
| // Occurrences it returns is the value resolveBillingReview is conditional on. |
| func (s *Store) billingReview(accountID int64, subscription, reason string) (billingReview, error) { |