amadan
Multi-currency: the fourteenth review's findings
A first purchase's record consumes its exact standing intent in the same transaction and records nothing when the intent is gone (Astra's P2: a reconciliation paused on the provider read while the operator resolved the review went on to record the retired purchase). The same lifecycle on every path: an intent under an open review is never retired by an expiry; a retired intent's completion and subscription events are settled, not retried, before any validation; an unknown terms version on a bound intent's subscription is the review; a refusal whose session read was answered with another files nothing. The fourteenth review's reports; dispositions "Review fourteen".
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 files changed,
+489
−29
docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review14.md+136 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+19 −3docs/superpowers/specs/OPUS-REVIEW-multi-currency-review14.md+115 −0internal/console/billingcurrency_test.go+146 −13internal/console/billingservice.go+42 −9internal/console/billingservice_test.go+9 −0internal/console/billingstore.go+22 −4
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review14.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review14.md| new file mode 100644 |
| index 000000000..d43e7bc4c |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review14.md |
| @@ -0,0 +1,136 @@ |
| +| Thirteenth-review finding | Status at `9574a798` | Would its regression pass without its fix? | |
| +|---|---|---| |
| +| **Opus P3-1 — invalid first-purchase plan, band or quantity gets no review** | **Closed for those errors.** They now call `refuse()` at `internal/console/billingservice.go:845`. The terms-version exception remains problematic below. | **No.** The quantity-2 and unknown-band cases at `billingcurrency_test.go:3589` require a review and 204 in both event orders and through the pass. | |
| +| **Opus P3-2 — refusal review leaves an open bound session payable** | **Closed for the reported successful-read case.** `refuse()` reads and expires it at `billingservice.go:810`. Its subsequent lifecycle remains defective. | **No.** `TestAHandMadeSubscriptionExpiresTheOpenBoundSession`, at `billingcurrency_test.go:3700`, requires expiry. It stops before the next pass, missing Finding 2. | |
| +| **Opus P3-3 — retired subscription events retry forever** | **Partly closed.** An otherwise valid subscription naming this account’s archived intent receives 204. Some permanently settled paths still return 503. | **No for the implemented case.** Removing the retirement check fails the two post-resolution subscription events at `billingcurrency_test.go:2937`. | |
| +| **Astra 1 — first-purchase validation bypasses the review** | **Partly closed.** Invalid plan/band/quantity are covered; a changed, unknown terms version still bypasses review. | **No for quantity/band.** The tests do not cover the remaining subscription-only terms change. | |
| +| **Astra 2 — currency-review regression is masked by price refusal** | **Closed for review routing.** The new case keeps the intent’s price and reaches the currency check at `billingservice.go:853`. | **No.** Replacing that branch’s `refuse()` with a bare refusal loses the required review. The new case changes both reported currencies; existing tests separately check subscription and item currency rejection. | |
| +| **Astra 3 — site amount test accepts another slot’s price** | **Partly closed.** The original static `bandCell` substitution is caught. Incorrect per-currency values can still pass. | **No for the original static substitution; yes for the counterexamples in Finding 6.** | |
| + |
| +**Thirteenth-review claims:** Opus’s ownership-comment correction is present, but related comments remain stale; the design correction is incomplete; “before the provider is asked” has been corrected in substance; the combined currency/price mutation claim is now qualified, but its new price-terms coverage claim is wrong; the expiry precedent is correctly qualified. Astra’s historical-agreement correction is present, while its broader review-coverage, ownership-comment and site-assurance objections remain partly open. Details follow under contradicted claims. Prose corrections have no behavioral test counterfactual. |
| + |
| +The pinned thirteenth reports both grade their numbered findings **P3**; they do not disagree on severity. On the earlier expiry dispute, **Opus’s conclusion is correct for an already-bound session**: expiry addresses the ID previously accepted and stored by the console. An unowned create or listing response supplies no equivalent authority and should remain untouched. |
| + |
| +All platform references below are at **`9574a798`**, against `12e70d95`, including the delta from `142293c5`. Site references are at **`5be6b41`**. These are source counterfactuals; no tests, mutations or network commands were run. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P2 — An in-flight reconciliation can activate a purchase after its intent has been retired.** |
| + |
| + **Where:** `internal/console/billingstore.go:591`, `:682`, `:745`; `internal/console/billingoperator.go:223`; `internal/console/billingservice.go:937`. |
| + |
| + **Scenario:** A completed purchase receives a refusal review because someone changed its subscription band. The metadata is corrected. Another reconciliation validates the standing intent and completed session, then pauses on `GetCustomer`. |
| + |
| + Meanwhile, the operator resolves the review. Resolution archives and deletes the intent and clears the hold, without changing the billing-account revision. It does not acquire the service’s billing lock. |
| + |
| + Reconciliation resumes. `applyBillingStateDated` accepts the unchanged revision, records the subscription and paid assignment, and queues entitlement publication. Its final deletion of the already-absent intent checks no affected-row count. The retired purchase has now been activated after resolution. |
| + |
| + **Smallest acceptable fix:** In the application transaction, require and atomically consume the exact standing intent when recording a new subscription. Refuse an absent, replaced or retired intent. A process-local lock alone is insufficient. |
| + |
| + The late-completion test at `billingcurrency_test.go:3010` would still pass with this defect: it begins the replay **after** resolution. Add a blocked customer read, resolve through the operator API, then resume and require no subscription, assignment or entitlement publication. |
| + |
| +2. **P3 — The new expiry path allows ordinary cleanup to delete the reviewed intent before resolution.** |
| + |
| + **Where:** `internal/console/billingservice.go:810`, `:633`; `internal/console/billingpass.go:209`; `internal/console/billingstore.go:450`, `:935`. |
| + |
| + **Scenario:** An intent requests Personal band A. A manually created subscription names that intent but carries band D metadata and the matching band-D price. The session itself remains unchanged and open. |
| + |
| + Reconcile reaches the intent comparison, expires the session and files its review. On the next pass, ordinary expired-session cleanup deletes the intent because `retireIntent` does not check for that open review. A checkout click can perform the same deletion and open another purchase while the first subscription remains unresolved. |
| + |
| + Resolution subsequently finds no intent and therefore writes no `billing_retired_intents` row. Later events for the self-consistent band-D subscription reach `sql.ErrNoRows` and return 503, even after resolution. |
| + |
| + **Smallest acceptable fix:** Prevent ordinary retirement from deleting an intent with an unresolved checkout-refusal review. Leave archival and deletion to the resolution transaction. |
| + |
| + The new test at `billingcurrency_test.go:3700` misses both consequences: it stops immediately after filing, and its price-only mutation continues failing the earlier price check. Extend it through another pass and resolution, using a changed band **and matching price**. |
| + |
| +3. **P3 — The unknown-terms exception retries a purchase that cannot match its standing intent.** |
| + |
| + **Where:** `internal/console/billingservice.go:832`, `:886`; `internal/plans/pricebook.go:506`. |
| + |
| + **Scenario:** A GBP purchase’s intent and completed session retain `2026-09-07-v1`. Only the subscription’s `terms_version` becomes `2031-01-01-v9`. |
| + |
| + Reconcile returns `ErrUnknownTermsVersion` before comparing the subscription with the intent. Both webhook orders return 503; the pass files no review. |
| + |
| + The deployment explanation does not justify this case. A newer binary learning the subscription’s terms cannot make them equal the intent’s different, immutable terms. Retrying is appropriate when the intent itself names an unknown version—not whenever the subscription does. |
| + |
| + **Smallest acceptable fix:** For a subscription naming the standing bound intent, compare its raw terms version with the intent before applying the unknown-version retry exception. A difference must use `refuse()`; an equal but unsupported version may retry. |
| + |
| +4. **P3 — A retired session’s completion still receives permanent 503 retries when no replacement intent stands.** |
| + |
| + **Where:** `internal/console/billingservice.go:1095`; `internal/console/billinghandlers.go:273`; retirement lookup at `billingservice.go:875`. |
| + |
| + **Scenario:** Resolve a paid, refused session and do not start another checkout. Deliver a new `checkout.session.completed` event for that session. |
| + |
| + The binder finds no standing intent and returns `sql.ErrNoRows`. The handler returns 503 without consulting the retirement record. The new retirement check exists only in subscription reconciliation. |
| + |
| + Retirement checking also occurs after terms and status validation, so an archived subscription with unknown terms still returns 503 before reaching it. |
| + |
| + **Smallest acceptable fix:** Recognize an account’s exact archived intent/session in the binder and acknowledge its rejection. In reconcile, check established retirement before validations whose retry can no longer produce a recordable purchase. Preserve retries for genuinely unbound, unretired purchases. |
| + |
| + The current late-completion test has a replacement intent standing and discards the HTTP status in its `send` helper (`billingcurrency_test.go:2874`, `:3020`), so it does not establish this behavior. |
| + |
| +5. **P3 — A mismatched checkout-read ID can produce a resolvable review without expiring the actual bound session.** |
| + |
| + **Where:** `internal/console/billingservice.go:814`. |
| + |
| + **Scenario:** While refusing the manually created subscription, `GetCheckout(cs_A)` returns `cs_B`. The conditional skips expiry because the IDs differ, but execution still files the review naming `cs_A`. |
| + |
| + The operator can resolve that review and retire A’s intent while `cs_A` remains open. This recreates the payable-after-resolution condition the new read was intended to prevent. |
| + |
| + **Smallest acceptable fix:** Treat a returned ID different from the bound ID as a failed read and stop before filing the resolution-enabling review. Never expire the returned foreign ID. Add this response mutation to the new refusal test. |
| + |
| +6. **P3 — The site’s new test still accepts incorrect amounts in particular currency slots.** |
| + |
| + **Where, site:** `test/pricing-pricebook.test.mjs:71`, `:170`, `:183`; `copy-review/pricing/build.mjs:179`. |
| + |
| + **Concrete surviving substitutions:** |
| + |
| + - Replace Free’s GBP value with the literal **`£22`**. Its EUR approved-string assertion and static USD assertions remain unchanged. The whitelist accepts £22 from Personal. |
| + - Replace Community band C’s USD value with **`$30/y per member`**, instead of `$3/y per member`. `startsWith("$3")` passes, and $30 is a valid Business price elsewhere in the book. |
| + |
| + The new static checks compare text with its own USD/band-A attribute. They do not establish every attribute’s correct relationship to the export. |
| + |
| + **Smallest acceptable fix:** Compare every plan/band/currency slot, including Free, with its complete expected exported amount and suffix. Keep the static checks. Literal provenance itself requires a source check or changed-book reconstruction; output equality cannot distinguish a correct literal from a derived value. |
| + |
| + I found no currently incorrect generated amount at the pinned site commit. |
| + |
| +**Other requested boundaries** |
| + |
| +- **A — GBP throughout the purchase.** `GET ?country=IN¤cy=gbp` produces band D in GBP: Personal **£7.70**, 29.75 awake hours, 0.7 GB storage and 0.7 GB transfer. India requires postal information. The signed form reaches `handleBillingCheckout`, which compares the effective currency before `startBillingCheckoutQuoted` (`billinghandlers.go:71`, `:102`, `:117`). Checkout validates the GBP unit amount, stores GBP/book on the intent, and sends GBP with Adaptive Pricing disabled (`billingservice.go:481`, `:513`, `:575`; `billingstripe/client.go:622`). Binding and reconcile check purchase identity before account/assignment writes. History and `AllowanceForMonth` retain those terms; version-2 calculations, invoice requests, recovery checks, hashes and quantity updates use the stored currency (`billinginvoicestore.go:530`; `billingcalculation.go:174`; `billinginvoice.go:859`, `:1024`; `billingquantity.go:1014`, `:1105`). I found no alternate-currency calculation or historical conversion. |
| + |
| +- **B/C/M/O/Q/U — Form and signatures.** Changed country or effective currency requires a quote. Forged agreeing fields cannot forge the server-keyed HMAC. It covers session token, account, book, country, band, currency, choice flag and member count (`billingpage.go:446`). The same tuple can be replayed; another tuple, session or rotated token cannot. All selectable plans were quoted; quantity follows membership and minimums; postal affects tax rather than the displayed pretax price. Membership is checked under the billing lock and again transactionally under the membership lock. Empty, repeated, padded, uppercase or unsupported posted currencies are refused on both doors after preceding authentication checks. An absent currency field computes the country default on a book console and EUR on a legacy console, still subject to signed-quote comparison. The quote door performs only an account-local encoded redirect. Held currency, including legacy EUR, overrides suggestions. Cookies cannot change an already-rendered form’s POST. Present-empty clears; absent falls back; invalid leaves the previous cookie unchanged, so that old preference can reappear on a later parameter-absent GET. |
| + |
| +- **D/L/R/W/Y/AA/DD/EE/FF/HH/JJ/KK/NN — Ownership and review.** `ownSession` checks mode, intent, both account identifiers, customer and bound ID (`billingservice.go:271`). Create/replay/listing paths do not accept or expire foreign responses. The pass and expected-intent binder attribute contradictory bound reads to the expected account. The webhook binder lacks that attribution authority; its already-recorded fast path returns existing state without recording a new purchase. Bound-ID expiry is justified; leaving unowned, unbound sessions open is also correct. Expiry failures return before filing and produce webhook 503 responses; completion winning the race can allow a later read to file without another expiry. Repeated failures need not resolve on the next read. Binder and reconcile use the same intent-keyed review, preventing duplicate occurrences while it stands. Expiry responses discarded by the bound-session helpers are not proof of retirement; provider-proof retirement separately checks ownership, account metadata, expired status and absence of payment/subscription. |
| + |
| +- **E/H/N/T — History barrier.** For valid stored history, the trigger matches `AllowanceForMonth`’s positive paid span: active, non-Free, intersecting the month and ending at the earliest next assignment, paid-through instant or month end (`billingstore.go:208`). Same-second supersession contributes zero duration. Free/inactive rows terminate predecessors. Renewal and quantity dating change `effective_at`; the assignment writer retains subscription `period_end` as paid-through. I found neither a valid old-build version-1 escape nor an erroneous refusal of this build’s legacy-only month. Replacement creation precedes transactional retirement of the old trigger; missing replacement causes failure without dropping the old one. Interruption can leave both, not neither; another successful start removes the old one. The successful-migration test does not prove the failure case. This protection does not make old-binary rollback generally safe. |
| + |
| +- **S/V/X/Z/BB/CC/GG/OO — Retirement and quantity history.** Resolution scopes its archive, quantity-event settlement and deletion to the reviewed account/intent. Review filing checks that exact intent still stands. References survive both readers and the API and can fill an older empty reference. `billingIntentRetired` is account-scoped; another account’s retirement cannot settle this account’s subscription. Randomly generated later intent IDs do not reuse the retired identity. `quantityOutcomesSQL` supplies the three admission checks, the processing guard is replaced transactionally, and no history-gap clearing sweep remains. The distinct-session late replay now detects recording of that completed retired session, but not the in-flight race in Finding 1. |
| + |
| +- **F/G/J/K — Figures, books and export.** Console inclusions divide pooled allowances by the actual quoted quantity, including unavailable Personal’s fallback quantity (`billingpage.go:395`). Current terms produce exact shares across supported quantities; band A matches the asserted approved sentences. Book validation checks matrices, rounding overflow, duplicate keys, countries and legacy EUR equality. Configuration permits complete historical catalogs and rejects shared price IDs; provisioning accepts at least the selected book’s 36 keys and matches the own-only currency-option rule. The export contains commercial definitions, not customer data or secrets. The site’s book matches it byte-for-byte. Current rates, country options/defaults and scaled inclusions agree. Draft build/check gates require exactly `PRICEBOOK_ALLOW_DRAFT=1`; the marker is first-line. Links always contain both parameters and forward currency only when chosen. A locale-only display can differ from the console’s eventual country default because it intentionally forwards no currency choice. |
| + |
| +**Claims the code contradicts** |
| + |
| +- **“The intent stands until resolution”** is false after the new successful expiry: ordinary cleanup removes it. **“Nothing records after resolution”** is also too broad because of the in-flight application race. See Findings 1–2. |
| + |
| +- **The design’s “refused for anything … terms”** at `docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md:152` conflicts with the raw unknown-terms return. The exception is acknowledged in dispositions `:275`, but its deployment justification overlooks the known, different standing-intent version. |
| + |
| +- **The price-terms mutation claim is wrong.** Dispositions `:257` says the unknown-band case reaches the price-terms refusal. Band is not a `PriceTerms` field. That case passes metadata validation and reaches `PriceFor` at `billingservice.go:839`, not the error branch at `:837`. |
| + |
| +- **Three ownership/read comments remain stale:** `billingservice.go:246` says reconcile names the session without reading it; `:1051` says the expected binder’s contradictory second read reviews nothing; `billingcurrency_test.go:3372` says the bound session is not expired. The implementations do otherwise. |
| + |
| +- **The site still overstates its guards.** Its test title at `test/pricing-pricebook.test.mjs:162` does not establish that no amount is typed into the script. Dispositions `:279` says rounded-rate fallbacks are gone, but `toFixed(2)` remains at site test `:186`. The claimed whole-token checks also permit commas: `build.mjs:145` accepts **“Minimum 10,000 members”** when the exported minimum is 10. Validate complete numeric tokens, including grouping separators, and each commercially meaningful occurrence. This concerns numeric validation, not approved wording. |
| + |
| +- **Review-three/four mutation qualifications are reasonable as source claims:** the default-as-choice mutation is outcome-equivalent under effective-currency comparison; provisioning tests inspect script text; successful migration does not establish failure ordering. Historical scratch-run counts cannot be verified from this source-only review. |
| + |
| +- **Test dependencies:** authentication, webhook freshness and fake expiry use real time (`billingservice_test.go:130`; `billingcurrency_test.go:1785`, `:2875`). I found no new calendar-date, test-order, temporary-root or shared-cookie-jar failure. The important missing tests are the interleavings and lifecycle continuations identified above. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +1. **Catalog and checkout matrix:** Verify all 36 price identities, expanded own-only currency options, exact amounts and quantities, tax behavior and disabled Adaptive Pricing. Complete USD/EUR/GBP purchases, including IN/GBP and Community GBP bands B/D. |
| +2. **Independent identity mutations:** Change subscription price, currency, item currency, quantity and metadata separately from session metadata. Deliver completion and subscription events in both orders and replay them. Valid purchases must record once; attributed refusals must produce one referenced review. |
| +3. **Expiry and completion races:** Establish which ownership fields Stripe permits changing, successful expiry’s returned shape, held-URL unusability, and idempotency behavior when completion wins. Failed reads/expiry must retry without falsely enabling resolution. |
| +4. **Resolution and recovery:** Lose create responses, recover by original-key replay and listing, resolve refusals, then deliver retired subscription and completion events with and without replacement intents. No retired purchase may activate or generate permanent retries. Exercise the in-flight resolution ordering from Finding 1. |
| +5. **Overage invoices:** Create, recover, finalize and pay in each currency. Check invoice/item/line currencies, exact allocations, floors, discounts, tax and house waivers. Replay legacy EUR request shapes. |
| +6. **Lifecycle and quantities:** Verify increases, proration, renewal, cancellation, quantity fencing and allowance dating. Establish customer-currency restrictions after cancellation and while old invoice items remain outstanding. |
| + |
| +**Verdict: reject.** |
| \ 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 5e3ca36e1..1637591f2 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -254,7 +254,7 @@ 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 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): |
| +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; review fourteen: an unknown band reaches `PriceFor`, not the price-terms check, so the price-terms case has its own test from the fourteenth-round commit, a book the console does not sell]; reconcile's and the binder's reads back to a plain ownership check): |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| @@ -273,10 +273,26 @@ Reports: `OPUS-REVIEW-multi-currency-review13.md`, `CODEX-FINDINGS-multi-currenc |
| | 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-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. [Review fourteen: the next pass then found the session expired and retired the intent while its review was open; from the fourteenth-round commit `retireIntent` retires nothing under an open review.] | `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` | | |
| +| 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 monthly-share fallback is gone [review fourteen: a two-decimal rate form remained and the inclusions' bands B to D were unpinned; site `a7dae7f` spells floors as prices and pins every band of every inclusion, recomputed from the export]. | 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. |
| + |
| +## Review fourteen (reviewed `9574a798`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review14.md`, `CODEX-FINDINGS-multi-currency-review14.md` (in the tree from this commit). Both found the thirteenth-round findings closed for the cases reported, with tests that fail without their fixes. Astra rejected on one P2 that is not wording: the record of a first purchase and the retirement of its intent were two transactions under no common lock, so a reconciliation paused on the provider read while the operator resolved the intent's review went on to record the purchase the operator had just retired. The rest of both lists is one lifecycle invariant seen from several paths: an intent under an open review stands, whatever its session's state, until the operator resolves it; a resolved (retired) intent can never again be recorded from, and every later event of its session or subscription is settled, not retried. Fixed, each with a test that fails without it; six mutants killed in a scratch copy (the consumption of the intent not required; `retireIntent` deleting under an open review; the unknown-terms exception made unconditional; the price-terms refusal back to a bare refusal; the binder's retired check removed; the refusal's read check removed): |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Astra 1 (P2): an in-flight reconciliation could record a purchase after the operator retired its intent (the resolution took no service lock and changed no revision; the record deleted the intent without counting rows). | Fixed in the database: for a first purchase the record transaction deletes the exact standing intent and refuses (`errBillingConflict`, nothing recorded, no entitlement queued, no event recorded) when it deleted none, so the record and the intent's consumption are one transaction and no process lock is relied on. | `billingstore.go` `applyBillingStateDated(consumeIntent)`, `billingservice.go` | `TestAResolutionDuringAReconciliationLeavesNothingRecorded` (the fake's customer read pauses the reconciliation while the operator resolves; the reconciliation returns a conflict; no state, assignment or event is recorded; the retirement record stands; later events are settled) | |
| +| Opus P3-1, Astra 2 (P3): the session `refuse()` had expired was retired by the next pass, so the intent stopped standing under its open review. | Fixed: `retireIntent` deletes nothing while a `billing_checkout_currency_refused` review of the intent is open; the resolution alone retires it, and writes the retirement record. | `billingstore.go` `retireIntent` | `TestAHandMadeSubscriptionExpiresTheOpenBoundSession` (a pass after the review; the intent stands; the resolution retires it with its record) | |
| +| Opus P3-2, Astra 3 (P3): a terms version edited on the subscription alone was a raw error (503 for ever, no review). | Fixed: when the subscription names the bound standing intent, an unknown terms version is the review (the intent's own terms are known here, so no later build could record it); with no such intent it stays a retry. | `billingservice.go` | `TestASubscriptionRefusedForItsPriceAloneIsReviewed` ("a terms version this build does not know", "a book the console does not sell") | |
| +| Astra 4 (P3): the completion of a retired intent's session, and a subscription whose retirement check came after validations, were 503 for ever. | Fixed: the binder settles a completion whose intent is in `billing_retired_intents`; reconcile checks the retirement first, before any validation whose retry could never record. | `billingservice.go` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` (the retired session's completion after the resolution, no replacement standing: 204) | |
| +| Astra 5 (P3): `refuse()` filed the review when its read of the bound session was answered with another session. | Fixed: another id is a failed read; nothing is filed or expired, and the provider tries again. | `billingservice.go` `refuse` | `TestARefusalWhoseSessionReadIsAnsweredWithAnotherFilesNothing` | |
| +| Opus P3-3, Astra 6 (P3): the site test left bands B to D of the inclusions unpinned, matched plan slots by prefix, left Free's currencies unpinned, admitted a two-decimal rate form, and the figure-token check found 10 in 10,000. | Fixed (site `f291a9e`): every band of every inclusion recomputed from the export; every plan, band and currency slot equal to its exported amount with the plan's suffix; Free's three currencies; floors spelt as prices and rates as they are; a token not followed by a grouping comma and digits. | site `test/pricing-pricebook.test.mjs`, `copy-review/pricing/build.mjs` | | |
| +| 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. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review14.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review14.md| new file mode 100644 |
| index 000000000..7f414c20c |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review14.md |
| @@ -0,0 +1,115 @@ |
| +# Fourteenth review of `feat/carloku-multi-currency-20261001` at `9574a798` (base `12e70d95`) |
| + |
| +**Verdict: accept with fixes.** All six findings from the thirteenth round are closed for the cases they reported. Each has a test that fails without its fix. This round changes nothing in how money is computed, sent to Stripe or stored. The delta adds three things: |
| +- a provider read and an expiry in reconcile's `refuse()`; |
| +- a read-only store query, `billingIntentRetired`; |
| +- a new `PriceFor` error branch in reconcile. |
| + |
| +Three P3 findings remain, plus five claims to correct. The most important: the session `refuse()` expires is retired by the next background pass, so the intent does not stand until the operator resolves the review, as the design says it does. |
| + |
| +I read source with `git show` only. I ran nothing and modified nothing; my scratch copies under `$TMPDIR` are deleted. |
| + |
| +## The thirteenth review's findings |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1 / Astra 1**: `PriceFor` errors returned raw, so no review and 503s | **Closed for plan, band and quantity.** `billingservice.go:845-847` sends `ErrInvalidQuantity`, `ErrUnknownPlan` and `ErrUnknownBand` to `refuse()`. **Unknown terms stay raw on purpose (`:834`); see P3-2.** | **No.** In "a quantity the plan does not sell" and "a band the console does not know" (`billingcurrency_test.go:3563-3599`), every `post` requires 204. A raw error is a 503 and fails the test in all three orders. | |
| +| **Opus P3-2**: reconcile's review did not read the session, so an open session survived resolution | **Closed for the webhook.** `refuse()` reads the bound id and expires it if open (`:810-818`). See P3-1 below for what the next pass then does. | **No.** `TestAHandMadeSubscriptionExpiresTheOpenBoundSession` requires `expireCalls == 1` and the session `expired`. Without the fix, nothing calls expire. | |
| +| **Opus P3-3**: a retired intent's subscription answered 503 for its whole life | **Closed** (`:875-879`, `billingstore.go:782-793`). | **No.** In the resolution test, the subscription's metadata, price and currency all match, so its events reach `pending == nil`. Without the fix that is `sql.ErrNoRows`, a 503. The test requires 204. | |
| +| **Astra 2**: the currency branch had no test of its own | **Closed.** "The right price in another currency" keeps `intent.Price` and changes only the subscription and item currency. The fake takes the session's currency from `money()` or `sessionCurrency` (`billingservice_test.go:162`), never from the subscription. So the binder passes the session and reconcile reaches `:854` alone. | **No.** A bare refusal there files no review in any order. | |
| +| **Astra 3**: the site test accepted any book amount by substring | **Closed for currency amounts.** Every static priced text must now equal its own USD or band-A value (`test/pricing-pricebook.test.mjs:166-181`). Amounts are matched as whole tokens. | **No** for Astra's `$28` example. **Partly yes** elsewhere: inclusion slots in bands B–D are still unpinned (P3-3), and one rounded-rate fallback remains (claim 4). | |
| +| **Claims** (both reviewers) | **Mostly corrected.** The design's checkout bullet, dispositions line 26, the twelfth summary and the pass comment are fixed. Two new contradictions follow from this round's code (claims 1 and 2). | n/a | |
| + |
| +**Where the two reviewers disagreed on severity:** |
| +- **The currency branch.** Opus filed it as a claim, Astra as a P3. The code shows Opus was right: the branch (`:853-855`) was correct all along, and only the test was missing. It was a test gap, not a defect. |
| +- **The site test.** Opus filed it as a note under MM, Astra as a P3. Both are fair. No page showed a wrong amount, but the claim that "no amount is typed in the script" was false. |
| +- **Unknown terms.** Opus's fix list excluded them; Astra's included them. Astra's broader reading is the closer one for a bound intent whose own terms this build knows (P3-2). |
| + |
| +## Findings |
| + |
| +### P3-1. The open session that `refuse()` expires is retired by the next pass, so the intent stops standing before the review is resolved |
| + |
| +- **Where:** `billingpass.go:207-210` (`expired` branch), then `billingstore.go:441-475`. `retireIntent` has no condition on an open review. |
| +- **Scenario:** |
| + 1. Intent I is bound to `cs_X`, which is still open. |
| + 2. A subscription is made by hand at Stripe, naming I at another band's price. |
| + 3. Reconcile's `refuse()` expires `cs_X` by its bound id and files the review (the new test stops here). |
| + 4. On the next pass, `GetCheckout(cs_X)` returns the intent's own session. The account, customer and metadata are all unchanged, so `ownSession` passes. |
| + 5. The session is `expired`, has no subscription and is unpaid. `expireResolvedIntent` therefore deletes I, and its quantity events are closed as `checkout_expired`. |
| +- **Result:** |
| + - The review is still open, but no intent stands. The buyer can open a second purchase while the hand-made subscription is still live at Stripe. |
| + - Resolving the review later writes no `billing_retired_intents` row (`billingstore.go:942-945` finds no intent). |
| + - This contradicts the design bullet (`:152`, "leaves the intent standing until the operator resolves it") and the dispositions' review-twelve and review-thirteen rows. |
| + - The same thing happens after `ownBoundSession`'s expiry if the contradicting field is later edited back (Opus KK called that case harmless). With `refuse()` it is the normal path, because nothing about the session itself contradicts. |
| +- **Smallest fix:** in `retireIntent`, delete only when no review is open for the intent. Add `AND NOT EXISTS (SELECT 1 FROM billing_reviews WHERE account_id=? AND subscription=<intent id> AND reason='checkout_refused' AND resolved_at=0)` to the `DELETE`, treating zero rows affected as `errBillingConflict` (which the pass already ignores). Then run a pass at the end of `TestAHandMadeSubscriptionExpiresTheOpenBoundSession` and assert the intent still stands. |
| + |
| +### P3-2. A terms version edited on the subscription alone answers 503 indefinitely, with no review, although a retry can never record it |
| + |
| +- **Where:** `billingservice.go:833-836`. |
| +- **Scenario:** |
| + 1. A GBP purchase completes on `cs_X`. |
| + 2. Before it is recorded, the subscription's `metadata.terms_version` is edited at Stripe. The session is left untouched. |
| + 3. The subscription event: `PriceTerms.Validate` returns `ErrUnknownTermsVersion` (`pricebook.go:507`), which is returned raw, so 503. |
| + 4. The completion event: the binder passes the session, then reconcile fails the same way, so 503. |
| + 5. Every pass fails the same way. No review is filed, and the account can neither buy nor be deleted. |
| +- **Why the comment's reason does not hold here:** "a newer build may know it" is only true when the intent was not made by this build. A newer build that knows the edited version reaches `intent.Terms != terms` at `:893` and refuses anyway. So a retry can only postpone the review, and only a deploy ends the 503s. The binder already reviews the same edit when it is on the session (`billingcurrency_test.go:2396`). |
| +- **Design contradiction:** the design (`:152`) lists "terms" among the refusals that file a review. This build knows exactly one terms version, so a terms edit is never reviewed here. |
| +- **Smallest fix:** at `:834`, call `refuse()` when `pending != nil && pending.ID == sub.Metadata["checkout_intent"] && pending.Terms == terms.Version`, and keep the raw error otherwise. Add a test case with `f.meta["terms_version"]` edited and `sessionMeta` copied. |
| + |
| +### P3-3. The site test leaves the band B–D values of seven of the ten inclusions unpinned |
| + |
| +- **Where:** site `test/pricing-pricebook.test.mjs:88-96`, against `copy-review/pricing/build.mjs:116-119`. |
| +- **What is pinned:** |
| + - All four bands of only `plan.personal.i2`, `plan.community.i2` and `plan.business.i4`. |
| + - The new slot test (`:174-180`) compares only band A. The amount whitelist sees only tokens with a currency sign, and inclusions have none. |
| +- **Scenario:** `build.mjs` hard-codes band D of Personal storage as `"0.75 GB of storage"` instead of the computed `0.7`. Static text and band A are unchanged, so every test passes. A visitor choosing India then sees an inclusion the console does not grant (the console computes `0.7`). |
| +- **Smallest fix:** loop over every banded `<li>`. For each band, compare against `swapFigure(S[id], …)` recomputed from `resources_per_member × band_factors_bps / 10000` in the test itself. Also check band A against `S[id]`. |
| + |
| +## The added sections |
| + |
| +**NN. Can `refuse()`'s read or expiry act on a session that is not this intent's?** |
| +- **Another session:** no. The id read and expired is always `pending.SessionID`, which the console bound only after `intentSession` passed. The read decides *whether* to expire and never supplies the id. The answer to the expiry is discarded. |
| +- **Completion in flight:** if the expiry loses the race, Stripe refuses it and `refuse()` returns the error, so 503 and nothing is filed. The completion's binder then binds and reconciles the real subscription. That records, and the intent is deleted (`billingstore.go:746`). The retried hand-made event then reaches `refuse()` with no intent standing, so a bare refusal and 204, with no review of the hand-made subscription. That is acceptable, because only someone at the provider can create it. |
| +- **Can both file?** |
| + - The review is filed once while open (`billingstore.go:976-1000`), so `refuse()` and the binder cannot file twice. |
| + - If `refuse()` files first (the session is already complete) and the real subscription then records, the review stays open while no intent stands. Its resolution retires nothing. Benign. |
| +- **What the webhook answers:** a failed read or expiry is a 503, and a retry is right. |
| + |
| +**OO. `billingIntentRetired`:** |
| +- **Another account's retired intent:** it is never settled under this account. The lookup is keyed by the account the subscription names (`:875`), so a subscription naming account B and A's intent finds nothing. It stays a 503 retry and records nothing. That is acceptable: it cannot be attributed to anyone. |
| +- **Reuse of a retired intent id:** impossible. Intent ids are random nonces (`billingstore.go:342`), and the table's key is `(account_id, intent_id)`. |
| +- **Is anything settled that should retry?** No. A retired intent can never stand again, so its subscription can never be recorded. Only operator resolution writes that table; expiry retirement does not. |
| +- **One residual case:** a retired subscription in `incomplete` status still gets `errBillingPending`, so 503s until it becomes `incomplete_expired`. |
| + |
| +**PP. The terms exception, and other 503s for a first purchase that will never succeed:** |
| +- **The terms exception:** right only when the intent's own terms are unknown to this build; see P3-2. |
| +- **Other permanent 503s I found:** |
| + - A subscription made by hand that exactly matches an intent whose session later expired on its own. The pass retires the intent without writing `billing_retired_intents`, so the subscription is `sql.ErrNoRows` indefinitely and never reviewed. This needs a hand-made subscription; see the Stripe list below. |
| + - Late events of an old subscription once a newer purchase is also dead: `pending` is nil and the intent is not retired, so 503. This was present before this branch. |
| + |
| +**QQ. The site test:** |
| +- **Pinned now:** every currency slot in every currency. That is the plan prices for all bands and currencies, the 12 table cells, the 4 rates for all currencies by parse-back, the floor in USD and GBP, and the EUR floor (which is the approved `over.p2` string). The static fallback is also checked slot by slot. |
| +- **Whole-token match:** correct. The lookbehind is `(^|[^0-9.])` and the lookahead is `(?![0-9.])`, so `€2` no longer matches inside `€24`. |
| +- **Still open:** P3-3 (inclusion slots), and the leftover `toFixed(2)` (claim 4). |
| + |
| +**Unchanged sections.** The delta does not touch `plans`, `cmd`, `billingstripe`, the page, the handlers, the cookies, the trigger, the quantity code or the export. Sections A–M and O–LL stand as rounds ten to thirteen traced them. One GBP purchase still has no conversion or re-pricing anywhere in its chain. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **`billingservice.go:245-247`** (`intentSession` comment): "the review reconcile files … names the bound session without reading it." `refuse()` now reads that session and may expire it (`:810-818`). |
| +2. **The design, `:152`**: "leaves the intent standing until the operator resolves it" is false after `refuse()`'s expiry (P3-1). "Refused for anything (… terms …)" is false for a terms edit (P3-2). |
| +3. **Dispositions, review twelve, the bracketed note**: "the price-terms case is reached by the unknown-band case." This is false. `priceTermsFromMetadata` validates terms, book and currency, but not band (`pricebook.go:506-530`). An unknown band reaches the `PriceFor` branch (`:846`). The price-terms `refuse()` at `:837` still has no first-purchase test with the session intact, so reverting it to a bare refusal survives every test. |
| +4. **Dispositions, review thirteen, Astra 3**: "the rounded-rate … fallbacks are gone." The site test's `micro` (`:186`) still admits `toFixed(2)` of every overage rate, for example `$0.13` for storage's `0.1272`. The rate slots themselves are pinned exactly, so this only matters for a token outside them. |
| +5. **Dispositions, review thirteen, Opus P3-2 row**: "so nothing can be paid through it after the operator resolves." That holds for payment. But the intent is gone before the resolution, not because of it (P3-1). |
| + |
| +## Verify in Stripe test mode before enabling |
| + |
| +All earlier lists stand. Add: |
| + |
| +1. **A hand-made subscription with the session open, then let the pass run.** Confirm the expiry kills the URL. Before the P3-1 fix this run shows the intent retired by the pass while the review is open; after the fix, it should still stand. |
| +2. **Expire a session while its payment is in progress.** Check whether Stripe refuses the expiry (and what error it gives) or blocks the payment. This decides which branch of NN happens in practice. |
| +3. **Events of a resolved refusal's subscription.** Check `updated`, `deleted` and `invoice.paid`, plus an `incomplete` one. Expect one rejected-events row and 204 for each, and that Stripe stops retrying. |
| +4. **A terms version edited on the subscription only** (P3-2). Before the fix this shows 503 retries with no review. |
| +5. **Does `checkout.session` read `complete` by the time `customer.subscription.created` is delivered?** This confirms a legitimate subscription can never meet an `open` session in `refuse()`. |
| + |
| +**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 a299659c8..713196b98 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -2939,6 +2939,11 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| t.Fatalf("%s after the resolution: %d; want it settled with 204, not retried", typ, code) |
| } |
| } |
| + // And the retired session's own completion, with no replacement intent |
| + // standing: settled too (review fourteen, Astra 4). |
| + 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) |
| + } |
| 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) |
| @@ -3368,10 +3373,10 @@ func TestAGapFromAnObsoleteOutcomeListStaysHeld(t *testing.T) { |
| // The session an intent is bound to, read back as nobody's (its account |
| // metadata, client reference or customer changed at the provider), is a |
| -// contradiction a person must see: the intent-keyed review is filed with |
| -// the session as its reference, nothing is offered, expired or recorded, |
| -// the intent stands until the operator resolves the review, and then a new |
| -// purchase opens. (Review eleven, Opus P3-1: such a session wedged the |
| +// contradiction a person must see: the session is expired by the id the |
| +// console bound, the intent-keyed review is filed with it as reference, |
| +// nothing is offered or recorded, the intent stands until the operator |
| +// resolves the review, and then a new purchase opens. (Review eleven, Opus P3-1: such a session wedged the |
| // intent with no review, and could still be paid through a URL held.) |
| func TestABoundSessionReadBackAsNobodysIsReviewed(t *testing.T) { |
| fx, f, acct := newCurrencyFixture(t) |
| @@ -3560,11 +3565,13 @@ 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), |
| - "the right price in another currency": "", |
| - "a quantity the plan does not sell": "", |
| - "a band the console does not know": "", |
| + "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": "", |
| + "a terms version this build does not know": "", |
| + "a book the console does not sell": "", |
| } { |
| for _, order := range []string{"subscription first", "completion first", "the pass"} { |
| t.Run(name+", "+order, func(t *testing.T) { |
| @@ -3588,15 +3595,23 @@ func TestASubscriptionRefusedForItsPriceAloneIsReviewed(t *testing.T) { |
| 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. |
| + case "a band the console does not know", "a terms version this build does not know", "a book the console does not sell": |
| + // The subscription's own metadata edited to a value this |
| + // build does not sell or know; 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" |
| + switch name { |
| + case "a band the console does not know": |
| + f.meta["band"] = "Z" |
| + case "a terms version this build does not know": |
| + f.meta["terms_version"] = "2031-01-01-v9" |
| + default: |
| + f.meta["price_book"] = "2031-01-01-other" |
| + } |
| } |
| f.mu.Unlock() |
| post := func(id, typ, object string) { |
| @@ -3726,4 +3741,122 @@ func TestAHandMadeSubscriptionExpiresTheOpenBoundSession(t *testing.T) { |
| 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) |
| } |
| + // The next pass finds the session expired, unpaid and the intent's |
| + // own: still no retirement while the review is open (review fourteen, |
| + // Opus P3-1), so no second purchase opens beside the hand-made |
| + // subscription until the operator resolves. |
| + _ = fx.srv.billingPass(context.Background(), f.now.Add(2*time.Minute)) |
| + if standing, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || standing.ID != intent.ID { |
| + t.Fatalf("after a pass: intent %+v %v; want it standing under its open review", standing, err) |
| + } |
| + if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.resolveBillingReview(billingReviewResolution{AccountID: acct.ID, Subscription: intent.ID, Reason: billingCheckoutRefusedReason, Occurrence: 1, Actor: "paul@keymail.dev", Note: "cancelled at the provider"}, f.now.Add(10*time.Minute)); err != nil { |
| + t.Fatalf("resolve: %v", err) |
| + } |
| + var retired string |
| + if err := fx.st.db.QueryRow(`SELECT session_id FROM billing_retired_intents WHERE account_id=? AND intent_id=?`, acct.ID, intent.ID).Scan(&retired); err != nil || retired != intent.SessionID { |
| + t.Fatalf("the retirement record: %q %v; want the resolution to retire the intent it reviewed", retired, err) |
| + } |
| +} |
| + |
| +// A reconciliation paused between its checks and its record (the provider |
| +// being read) while the operator resolves the intent's review records |
| +// nothing: the record and the consumption of the exact standing intent are |
| +// one transaction, and an intent retired under it fails the record. |
| +// (Review fourteen, Astra 1, P2.) |
| +func TestAResolutionDuringAReconciliationLeavesNothingRecorded(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) |
| + } |
| + // Completed; the subscription's band and price changed to another |
| + // band's: the review is filed. |
| + 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_race_sub", "customer.subscription.created", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the mismatched subscription event: %d", code) |
| + } |
| + var open int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=? AND resolved_at=0`, acct.ID, intent.ID, billingCheckoutRefusedReason).Scan(&open); err != nil || open != 1 { |
| + t.Fatalf("fixture: %d open reviews (%v)", open, err) |
| + } |
| + // The metadata is corrected at the provider; the next reconciliation |
| + // passes every check and pauses on the customer read, during which the |
| + // operator resolves the review (no lock of the service's is held by |
| + // the resolution). |
| + if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil { |
| + t.Fatal(err) |
| + } |
| + f.mu.Lock() |
| + f.meta["band"], f.subscriptionPrice = intent.Band, intent.Price |
| + f.customerHook = func() { |
| + if err := fx.st.resolveBillingReview(billingReviewResolution{AccountID: acct.ID, Subscription: intent.ID, Reason: billingCheckoutRefusedReason, Occurrence: 1, Actor: "paul@keymail.dev", Note: "refunded"}, f.now.Add(time.Hour)); err != nil { |
| + t.Errorf("resolve during the reconciliation: %v", err) |
| + } |
| + } |
| + f.mu.Unlock() |
| + err = fx.srv.reconcileBillingSubscription(context.Background(), "sub_paid", "evt_race_recon", f.now.Add(2*time.Minute)) |
| + if !errors.Is(err, errBillingConflict) { |
| + t.Fatalf("the reconciliation that lost its intent: %v; want a conflict and nothing recorded", err) |
| + } |
| + if f.customerHook != nil { |
| + t.Fatal("fixture: the customer read never ran") |
| + } |
| + state, _ := fx.st.billingAccount(acct.ID) |
| + var assignments, queued int |
| + _ = fx.st.db.QueryRow(`SELECT count(*) FROM billing_book_assignments WHERE account_id=?`, acct.ID).Scan(&assignments) |
| + _ = fx.st.db.QueryRow(`SELECT count(*) FROM billing_events WHERE id='evt_race_recon'`).Scan(&queued) |
| + if state.Subscription != "" || assignments != 0 || queued != 0 { |
| + t.Fatalf("state %+v, %d assignments, %d events recorded; want nothing recorded from a retired intent", state, assignments, queued) |
| + } |
| + if _, err := fx.st.billingCheckoutIntent(acct.ID); err == nil { |
| + t.Fatal("the intent stands after its resolution") |
| + } |
| + var retired string |
| + if err := fx.st.db.QueryRow(`SELECT session_id FROM billing_retired_intents WHERE account_id=? AND intent_id=?`, acct.ID, intent.ID).Scan(&retired); err != nil || retired != intent.SessionID { |
| + t.Fatalf("the retirement record: %q %v", retired, err) |
| + } |
| + // Every later event of that subscription is settled. |
| + if code := f.webhookPost(t, fx, "evt_race_after", "customer.subscription.updated", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("after the resolution: %d; want 204", code) |
| + } |
| +} |
| + |
| +// reconcile's refusal reads the bound session before it names it: a read |
| +// answered with another session files nothing and expires nothing (the |
| +// provider tries again). (Review fourteen, Astra 5.) |
| +func TestARefusalWhoseSessionReadIsAnsweredWithAnotherFilesNothing(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) |
| + f.mu.Lock() |
| + f.subscriptionPrice = currencyPriceID(plans.PlanPersonal, plans.BandD, plans.GBP) |
| + f.sessionAnswers = []func(*fakeBillingStripe){func(f *fakeBillingStripe) { f.sessionID = "cs_other" }} |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_other_read", "customer.subscription.created", "sub_paid"); code != http.StatusServiceUnavailable { |
| + t.Fatalf("the subscription event whose session read was answered with another: %d; want 503 (try again)", code) |
| + } |
| + var open int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=?`, acct.ID, intent.ID, billingCheckoutRefusedReason).Scan(&open); err != nil || open != 0 { |
| + t.Fatalf("%d reviews (%v); want none filed on a session that was not read", open, err) |
| + } |
| + if f.expireCalls != 0 { |
| + t.Fatalf("%d expiries; want none on another session's id", f.expireCalls) |
| + } |
| } |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index 2f9824d39..b4364f926 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -244,7 +244,8 @@ func (s *Server) checkoutParams(acct *Account, intent billingIntent) billingstri |
| // records nothing; a bound session that fails ownSession, which is |
| // reviewed and expired for that very reason (ownBoundSession); and the |
| // review reconcile files for a subscription that is not its intent's |
| -// purchase, which names the bound session without reading it. |
| +// purchase, which reads the bound session only to expire it if it is |
| +// still open, and names it. |
| func (b *billingRuntime) intentSession(session *billingstripe.Session, intent billingIntent, acct *Account) bool { |
| if !b.ownSession(session, intent, acct) { |
| return false |
| @@ -786,8 +787,7 @@ 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; not a terms version this build does not know, |
| - // which a newer build may, and so stays a retry), is the intent-keyed |
| + // band, its quantity, its terms version), 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. |
| @@ -798,6 +798,16 @@ 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 |
| + } |
| + } |
| } |
| refuse := func() error { |
| if pending != nil && pending.SessionID != "" && pending.ID == sub.Metadata["checkout_intent"] { |
| @@ -811,7 +821,14 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| if err != nil { |
| return err |
| } |
| - if session.ID == pending.SessionID && session.Status == "open" { |
| + if session.ID != pending.SessionID { |
| + // Answered with another session: the read failed, and no |
| + // review is filed on a session that was not read (a |
| + // resolution would retire the intent with its own session |
| + // still open). The provider tries again. |
| + return errBillingPending |
| + } |
| + if session.Status == "open" { |
| if _, err := b.client.ExpireCheckout(ctx, pending.SessionID, "carloku-expire-"+pending.ID); err != nil { |
| return err |
| } |
| @@ -831,7 +848,12 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| // recorded as anything. |
| priceTerms, err := priceTermsFromMetadata(sub.Metadata) |
| if err != nil { |
| - if errors.Is(err, plans.ErrUnknownTermsVersion) { |
| + // 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. |
| + if errors.Is(err, plans.ErrUnknownTermsVersion) && !(pending != nil && pending.SessionID != "" && pending.ID == sub.Metadata["checkout_intent"]) { |
| return err |
| } |
| return refuse() |
| @@ -1012,7 +1034,7 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| quantityStart, quantityKey, quantityConfirmed = time.Unix(op.CreatedAt, 0).UTC(), op.Key, op.ConfirmedAt != 0 |
| } |
| } |
| - return s.store.applyBillingStateDated(next, state.Revision, eventID, effective, renewalStart, quantityStart, quantityKey, quantityConfirmed) |
| + return s.store.applyBillingStateDated(next, state.Revision, eventID, effective, renewalStart, quantityStart, quantityKey, quantityConfirmed, state.Subscription != id) |
| } |
| // provableRenewal reports whether current -> next is exactly one uninterrupted |
| @@ -1048,9 +1070,10 @@ func (s *Server) bindBillingCheckout(ctx context.Context, sessionID string) (str |
| // known intent (the pass): the session it reads back must be that |
| // intent's own, by the intent's account, before any account is derived |
| // from the response. A second answer that names another account's |
| -// everything under the same id binds, reviews and records nothing. The |
| -// webhook, which knows no intent, passes nil and is judged against the |
| -// intent the response names. |
| +// everything under the same id binds and records nothing, and, being the |
| +// session the intent is bound to, is expired by that id and reviewed |
| +// (ownBoundSession). The webhook, which knows no intent, passes nil and |
| +// is judged against the intent the response names. |
| func (s *Server) bindBillingCheckoutFor(ctx context.Context, sessionID string, expected *billingIntent) (string, error) { |
| b := s.billing |
| if b == nil { |
| @@ -1093,6 +1116,16 @@ func (s *Server) bindBillingCheckoutFor(ctx context.Context, sessionID string, e |
| return session.SubscriptionID, nil |
| } |
| intent, err := s.store.billingCheckoutIntent(acct.ID) |
| + if errors.Is(err, sql.ErrNoRows) { |
| + // No intent stands. The completion of a session whose intent the |
| + // operator retired (a refusal resolved) is settled, not retried: |
| + // nothing can ever be recorded from it. |
| + if retired, rerr := s.store.billingIntentRetired(acct.ID, session.Metadata["checkout_intent"]); rerr != nil { |
| + return "", rerr |
| + } else if retired { |
| + return "", errBillingIdentity |
| + } |
| + } |
| if err != nil { |
| return "", err |
| } |
diff --git a/internal/console/billingservice_test.go b/internal/console/billingservice_test.go| index 820e9d617..ab34a45e3 100644 |
| --- a/internal/console/billingservice_test.go |
| +++ b/internal/console/billingservice_test.go |
| @@ -59,6 +59,10 @@ type fakeBillingStripe struct { |
| // place of meta: the provider answering an authorised expiry with a |
| // session that names another account. |
| expireMeta map[string]string |
| + // customerHook, when set, runs inside the customer read, once: what |
| + // happens while a reconciliation is paused between its checks and its |
| + // record (an operator resolving a review, say). |
| + customerHook func() |
| // sessionAnswers, when set, are applied to the fake in order, one per |
| // rendering of the session, before it is rendered: the provider |
| // answering successive reads of one session differently. |
| @@ -204,6 +208,11 @@ func (f *fakeBillingStripe) serve(w http.ResponseWriter, r *http.Request) { |
| case r.URL.Path == "/v1/billing_portal/configurations/bpc_safe": |
| out = map[string]any{"id": "bpc_safe", "active": true, "livemode": false, "features": map[string]any{"customer_update": map[string]any{"enabled": false}, "subscription_update": map[string]any{"enabled": false}, "subscription_cancel": map[string]any{"enabled": true, "mode": "at_period_end"}, "payment_method_update": map[string]bool{"enabled": true}, "invoice_history": map[string]bool{"enabled": true}}} |
| case r.URL.Path == "/v1/customers" || r.URL.Path == "/v1/customers/cus_paid": |
| + if r.Method == http.MethodGet && f.customerHook != nil { |
| + hook := f.customerHook |
| + f.customerHook = nil |
| + hook() |
| + } |
| if r.Method == http.MethodPost { |
| f.customerCalls++ |
| _ = r.ParseForm() |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index 47d7f8910..2c57004c8 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -447,7 +447,13 @@ func (s *Store) retireIntent(intent billingIntent, proof *billingstripe.Session, |
| return err |
| } |
| defer tx.Rollback() |
| - r, err := tx.Exec(`DELETE FROM billing_checkout_intents WHERE account_id=? AND id=? AND session_id=? AND live_mode=?`, intent.AccountID, intent.ID, intent.SessionID, intent.LiveMode) |
| + // An intent under an open review stands until the operator resolves |
| + // it, whatever its session's state: the review may name a session the |
| + // console itself expired (ownBoundSession, reconcile's refusal), and |
| + // an expiry is then no proof that nothing was bought on the intent. |
| + r, err := tx.Exec(`DELETE FROM billing_checkout_intents WHERE account_id=? AND id=? AND session_id=? AND live_mode=? |
| + AND NOT EXISTS (SELECT 1 FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=? AND resolved_at=0)`, |
| + intent.AccountID, intent.ID, intent.SessionID, intent.LiveMode, intent.AccountID, intent.ID, billingCheckoutRefusedReason) |
| if err != nil { |
| return err |
| } |
| @@ -518,7 +524,7 @@ func (s *Store) applyBillingState(next billingState, expectedRevision int64, eve |
| // effective as usual, and a live change files a durable billing review: its |
| // month's allowance is then refused as incomplete rather than guessed. |
| func (s *Store) applyBillingStateAt(next billingState, expectedRevision int64, eventID string, effective, renewalStart time.Time) error { |
| - return s.applyBillingStateDated(next, expectedRevision, eventID, effective, renewalStart, time.Time{}, "", false) |
| + return s.applyBillingStateDated(next, expectedRevision, eventID, effective, renewalStart, time.Time{}, "", false, false) |
| } |
| // billingClockTolerance is how far ahead of this process's clock an |
| @@ -551,7 +557,13 @@ func billingQuantityDateReviewReason(at int64) string { |
| // success. Without it the quantity only matches the operation's target, |
| // which someone else could have set at another time, so the instant is |
| // never used: the assignment is dated when observed and the review filed. |
| -func (s *Store) applyBillingStateDated(next billingState, expectedRevision int64, eventID string, effective, renewalStart, quantityStart time.Time, quantityKey string, confirmed bool) error { |
| +// applyBillingStateDated records a subscription's state. For a first |
| +// purchase (consumeIntent) the record and the retirement of the intent it |
| +// was bought on are one transaction: the exact standing intent is deleted |
| +// here, and if it is gone (retired by an operator's resolution while the |
| +// provider was being read, or replaced) nothing is recorded and |
| +// errBillingConflict says so. No lock outside the database is relied on. |
| +func (s *Store) applyBillingStateDated(next billingState, expectedRevision int64, eventID string, effective, renewalStart, quantityStart time.Time, quantityKey string, confirmed, consumeIntent bool) error { |
| if next.AccountID <= 0 || next.Customer == "" || next.Subscription == "" || next.Quantity <= 0 || next.Plan == "" || next.Terms == "" || next.Band == "" || next.PeriodEnd <= next.PeriodStart { |
| return errors.New("billing_state_invalid") |
| } |
| @@ -743,8 +755,14 @@ func (s *Store) applyBillingStateDated(next billingState, expectedRevision int64 |
| } |
| } |
| if next.IntentID != "" { |
| - if _, err := tx.Exec(`DELETE FROM billing_checkout_intents WHERE account_id=? AND id=?`, next.AccountID, next.IntentID); err != nil { |
| + r, err := tx.Exec(`DELETE FROM billing_checkout_intents WHERE account_id=? AND id=?`, next.AccountID, next.IntentID) |
| + if err != nil { |
| + return err |
| + } |
| + if n, err := r.RowsAffected(); err != nil { |
| return err |
| + } else if consumeIntent && n != 1 { |
| + return errBillingConflict |
| } |
| } |
| if err := refreshQuantityChangeTx(tx, next.AccountID, queuedAt); err != nil { |