carlos / platform Public

Multi-currency: the eleventh review's findings

The session an intent is bound to, read back as nobody's, files the
intent-keyed review with the session as reference instead of wedging the
intent in silence (ownBoundSession, on every read of the bound session);
a subscription that names the intent and is not its purchase is the same
review once the intent is bound; an expiry's answer must name the
account in its metadata before the store retires anything; the fake's
expiry answer names the session asked for. Tests for each, the two
proof fixtures carry the account metadata the console always sets, and
the eleventh review's reports; dispositions corrected and "Review
eleven".

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

claude 3 weeks ago 781bb1c249e5744289b4709b997b39169faebb0c parent 28dc262

10 files changed, +511 −32

docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review11.md +74 −0
docs/superpowers/specs/DISPOSITIONS-multi-currency.md +18 −3
docs/superpowers/specs/OPUS-REVIEW-multi-currency-review11.md +123 −0
internal/console/billingcurrency_test.go +195 −5
internal/console/billingmembership_test.go +1 −1
internal/console/billingpass.go +4 −2
internal/console/billingquantityaudit_test.go +6 −1
internal/console/billingservice.go +77 −18
internal/console/billingservice_test.go +12 −1
internal/console/billingstore.go +1 −1

CI passed — run details

diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review11.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review11.md

new file mode 100644

index 000000000..f2fd43ca2

--- /dev/null

+++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review11.md

@@ -0,0 +1,74 @@

+| Tenth-review item | Status at `25487506` | Would its test pass without the fix? |

+|---|---|---|

+| **Opus P3-1: reconcile and binder disagree** | **Closed.** Reconcile now calls `intentSession` after ownership and files the same intent-keyed review as the binder. See `internal/console/billingservice.go:795`, `:805`, `:980`. | **No.** `TestASubscriptionEventBeforeTheCompletionMeetsTheSameJudgement`, at `internal/console/billingcurrency_test.go:3071`, changes session metadata independently of subscription metadata. Without the reconcile fix, the first subscription event records the purchase instead of leaving one review and the intent standing. |

+| **Opus P3-2: rollback note promises removed healing** | **Closed.** `docs/superpowers/specs/DISPOSITIONS-multi-currency.md:70` now says the gap remains held. | No executable test for the prose. The restart tests protect the retained holds, not the wording. |

+| **Opus claims about dispositions and coverage** | **Partly closed.** Historical summaries are substantially corrected, but the claim about testing the resolution’s intent predicate remains inaccurate; see below. | The resolution test **still passes** if its final delete is scoped only by account. |

+| **Astra 1: subscription event bypasses the binder’s judgement** | **Closed**, for the same code and distinguishing test as Opus P3-1. | **No.** |

+| **Astra 2: conflicting account metadata passes ownership** | **Closed on create, resume, listing, pass, binding and initial reconciliation; incomplete across expiry responses.** `ownSession` now checks both account identifiers at `internal/console/billingservice.go:265`. Finding 1 below is the remaining path. | **No** for the added ownership, resume and listing tests. They do not exercise contradictory metadata returned by `ExpireCheckout`. |

+| **Astra claim: single-review reader drops `reference`** | **Closed.** The reader selects and scans it at `internal/console/billingstore.go:786`. | **No.** The assertion at `internal/console/billingcurrency_test.go:2882` fails without it. |

+| **Astra claim: late-completion test replays the replacement session** | **Closed.** The replacement is `cs_test_second`; the replay returns the retired `cs_test_paid`, completed in EUR with its old metadata. See `internal/console/billingcurrency_test.go:2952`, `:2986`. | The former test could pass without exercising retirement. The revised test **cannot pass if that completion records billing state**, because `:2994` checks that no subscription was recorded; it also checks the replacement intent remains unchanged. This does not independently prove every guard along that path. |

+| **Astra claim: site test falsely claims export equality** | **Closed as a description correction.** At site commit `3f1610d`, `test/pricing-pricebook.test.mjs:32` accurately describes its schema/source-note checks. | **Yes.** Renaming the test changes no assertions. Separately, I compared the two committed exports: they are byte-identical. |

+These assessments follow the tenth-review inventory in the dispositions. **Neither tenth-review report exists in the Git tree at `25487506`**, so I could not independently inspect their complete claims or severity arguments. The inventory records the shared reconcile finding as P3 for both reviewers—not a severity disagreement. P3 fits the demonstrated code defect: the subscription still matches the configured purchase price, but contradictory session metadata previously escaped the review path.

+All test outcomes above are conclusions from source, not executed tests. Nothing was modified; no tests, network commands or AWS commands were run.

+**Findings, most severe first**

+1. **P3 — Expiry responses bypass the new account-metadata ownership check.**

+ **Locations:** `internal/console/billingservice.go:304`, `:308`, `:576`, `:580`; `internal/console/billingstore.go:442`, `:465`.

+ **Scenario:** Account A’s session passes ownership. Either its purchase fields are wrong, or the buyer changes the requested purchase, so the console legitimately requests expiry. The provider answers with the same session ID, `status=expired`, the correct intent, customer and `client_reference_id=A`, but `metadata.account_sqid=B`.

+ Both service paths accept that response without calling `ownSession` again. `retireIntent` checks the other ownership fields but omits `account_sqid`, then commits deletion of A’s intent and closes its quantity events. The replacement-purchase path can subsequently open another intent.

+ Thus a session that the new predicate explicitly defines as nobody’s is still accepted as retirement evidence. The initial expiry request was authorized; this finding concerns trusting its contradictory response, not an ability to expire an arbitrary unrelated session.

+ **Smallest acceptable fix:** Apply the complete ownership predicate to each expiry response before retirement, retaining the expired/unpaid/no-subscription checks. Add a regression where only the expiry response’s account metadata differs. Assert that the intent and pending quantity events remain intact and no replacement checkout opens.

+ The current fake cannot expose this through `sessionMeta`: its expiry response is separately constructed from `f.meta` at `internal/console/billingservice_test.go:261`.

+I found no additional P1/P2 defect in the currency computation or persistence paths inspected.

+The requested end-to-end and boundary checks support the following conclusions:

+- **GBP purchase and invoices:** `GET ?country=IN&currency=gbp` renders Personal at **£7.70**, band D. The signed form carries that country, currency, choice and member count. The handler checks the effective purchase currency; checkout stores GBP and the book, selects the corresponding price, and disables Adaptive Pricing. Binding and initial reconciliation judge the same purchase identity. Account and assignment writes retain GBP; monthly allowance selects those terms; calculation version 2, invoice requests, recovery checks and quantity-price lookup use the stored currency. See `billinghandlers.go:102`, `billingservice.go:423`, `:528`, `billingstore.go:599`, `:725`, `billingcalculation.go:174`, `billinginvoice.go:898`, `:1024`, and `billingquantity.go:1014` under `internal/console/`.

+- **One form, signatures and cookies:** The server-derived HMAC covers session token, account, book, country, band, currency, choice flag and members (`internal/console/billingpage.go:446`). Disclosed form values do not reveal its key. A different quote, session, account, book or token cannot reuse the signature. Reusing an unchanged quote from an older tab is allowed: it still buys what that tab displayed, subject to the current member-count check. Plan is selected from the displayed rows; quantity follows the signed member count and plan minimum; postal affects tax, while the page explicitly quotes before VAT (`templates/billing.html:42`). Present empty, malformed or repeated currency fields are refused; absence means no explicit choice. Authentication and consent checks precede checkout’s currency validation. The quote door constructs only the account’s billing URL and performs no purchase operation (`billinghandlers.go:137`, `:176`, `:197`). Present-empty GET parameters clear cookies; absent parameters may reuse them; invalid parameters leave existing cookies unchanged. Consequently an old cookie can reappear after an **invalid** URL followed by an absent parameter, but not after a successfully cleared cookie. POST checkout does not read these suggestion cookies.

+- **Held currency and inclusions:** A previous purchase, including legacy EUR, restricts the book-console form to its held currency (`internal/console/billingpage.go:301`); the service refuses another currency at `billingservice.go:515`. Inclusions use the pooled allowance divided by the charged quantity, including the unavailable Personal row’s fallback quantity (`billingpage.go:408`, `:427`). Current resource figures scale exactly in base units for every supported band, so increasing team quantity does not introduce fractional pooling discrepancies. Band A’s rendered sentences are checked verbatim at `billingcurrency_test.go:2628`.

+- **Trigger, migration and holds:** For valid stored histories, I found no span counterexample. The trigger and `AllowanceForMonth` both require an active non-free assignment with positive overlap ending at the next assignment, paid-through instant or month end (`internal/console/billingstore.go:208`; `internal/plans/month.go:146`). An active book assignment superseded by a cancellation in the same second contributes zero; a later legacy-only calculation is therefore permitted. Renewal and quantity dating alter `effective_at`; `period_end` remains the subscription’s paid-through endpoint copied by the sole book-assignment writer (`billingstore.go:618`, `:655`, `:725`). Migration creates the new trigger before transactionally checking its existence and dropping the old one (`:797`). A crash can temporarily leave **both**, but that sequence does not leave neither; a missing new trigger makes retirement fail without dropping the old one. Repeated startup is idempotent. This is still not general permission to roll back an older binary over book history. The outcome validators share `quantityOutcomesSQL`, and no production statement found clears an existing history gap; summary writes preserve it (`billingquantityaudit.go:61`; `billingmembership.go:462`).

+- **Refusal review and retirement:** Filing checks that the exact intent still exists; resolution selects, records and deletes exactly that intent, closes its events and preserves the refused session reference (`internal/console/billingstore.go:922`, `:940`, `:963`). Repeated binder/reconcile observations share one open review. The reference reaches both readers and the operator API. Apart from Finding 1, conflicting account identifiers prevent the ownership-dependent actions inspected. An already-issued URL can remain open at Stripe when ownership becomes contradictory, and the intent remains pending. Declining to expire that session is the correct call: the console lacks consistent authority over it.

+- **Book, export and site:** Book validation checks complete price matrices, rounding, overflow and legacy EUR agreement. Configuration requires the selected catalog and permits complete additional catalogs, with unique price IDs (`internal/plans/pricebook.go:231`; `cmd/console/billing.go:134`). The provisioning verifier’s own-only `currency_options` expression matches the console’s rule. The public export contains commercial data, not credentials or customer records. At site `3f1610d`, exact rates, band-scaled inclusions, country options and their default currencies come from the export; draft handling requires exactly `PRICEBOOK_ALLOW_DRAFT=1` and checks the first-line marker. Signup links always carry both parameters. Locale-only GBP can become the console’s default USD when no country or currency was chosen; that is the specified suggestion behaviour, and checkout requires its own quote before purchase.

+**Claims the code still contradicts**

+- **The review-four qualification still overstates the delete-predicate test.** `docs/superpowers/specs/DISPOSITIONS-multi-currency.md:108` says the predicate is tested by the first resolution. That fixture contains only the refused intent, so an account-only delete produces the same result. The second resolution returns at `internal/console/billingstore.go:891`, before reaching deletion. The accurate statement is already present at dispositions `:168`: this mutation survives under the tested invariants. Replace the qualification with that statement.

+- **The review-six mutation count remains inconsistent.** Dispositions `:156` says six mutants were killed; `:168` describes five killed and the account-only delete surviving. Correct the summary count. I cannot verify historical scratch-run claims without their artifacts; source inspection does not establish those runs occurred.

+- **“Every session … passes ownSession first” is too broad.** Beyond Finding 1, the already-recorded-subscription fast path at `internal/console/billingservice.go:964` returns before looking up an intent or calling `ownSession`. It relies on the account reference checks and stored subscription/customer identity. I found no new purchase-recording bypass there, but `billingservice.go:234` should describe this replay exception.

+- **The renamed site equality test remains a provenance check, not an equality gate.** Its corrected name is honest. The committed exports match now; changing an amount only in the site’s copy would not be detected by that particular test.

+The browser fixtures use explicit cookies rather than a shared jar. Relevant HTTP fixtures use `time.Now()` for authentication and webhook freshness and relative times for session expiry; those checks remain wall-clock-dependent. I found no new calendar-date, ordering or temporary-root failure in the reviewed delta.

+**What must be verified against Stripe test mode before enablement**

+1. **Catalog and fixed-currency checkout:** Exercise all 36 price identities, including GBP Community B/D rounding. Read expanded `currency_options`; verify own-only shapes, amounts, metadata, annual licensed pricing and the absence of extra currency options.

+2. **Payment presentation and completion:** Complete USD, EUR and GBP purchases with differing billing countries and browser locales. Confirm charged/presented currency, Adaptive Pricing disabled, no conversion, matching subscription/item currency, and actual checkout line price and quantity.

+3. **Provider mutability and event ordering:** Establish which session fields and line items Stripe permits changing before and after completion. Deliver subscription and completion events in both orders and replay them. Correct purchases must record once; owned inconsistent completions must leave one referenced review and the intent standing.

+4. **Recovery and expiry:** Lose create responses, replay the original idempotency key, recover through customer-scoped listing, and expire owned open sessions. Verify returned identity fields and completed-session expiry refusal. Confirm delayed completion after operator retirement cannot record a purchase or disturb a newer intent.

+5. **Monthly overages:** Create, recover, finalize and pay invoices in each currency. Verify invoice/item/line currencies, exact subtotal allocation, currency-specific floor boundaries, tax, house waivers and unchanged legacy EUR recovery requests.

+6. **Quantity and customer lifecycle:** Exercise increases, renewal changes and the quantity fence in each currency. Verify returned item price/currency and allowance dating. Establish Stripe’s restrictions for customers with active, canceled or incomplete-expired subscriptions and outstanding invoice items; keep currency changes refused until that behaviour is established.

+**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 568ca6c11..c08cfa4be 100644

--- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md

+++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md

@@ -105,7 +105,7 @@ The fixes are in the commit that adds this section and have not been read by a r

## Review four (at 8958392b; Opus accepted with fixes, Astra rejected)

-Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency-review4.md`. Both found the third round's code changes present and no path that stores or sends money in a currency other than the purchase's (Astra marked ownership, the crafted form and the trigger migration partly closed, which its findings were about); the disputed trigger reading went Astra's way. Every code finding of either that is marked fixed below has a test that fails without it, with three qualifications review ten named: the provisioning script's checks are tested by reading the script's text, the trigger's create-then-drop order is tested by a migration that succeeds (not by one that would fail in the other order), and a repeated resolution returns before its delete, so the delete's intent predicate is tested by the first resolution alone; eight mutants, one per fix, were killed in a scratch copy at the time (the runs are not reproducible from this file); two items are left open and say so.

+Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency-review4.md`. Both found the third round's code changes present and no path that stores or sends money in a currency other than the purchase's (Astra marked ownership, the crafted form and the trigger migration partly closed, which its findings were about); the disputed trigger reading went Astra's way. Every code finding of either that is marked fixed below has a test that fails without it, with three qualifications review ten named: the provisioning script's checks are tested by reading the script's text, the trigger's create-then-drop order is tested by a migration that succeeds (not by one that would fail in the other order), and the resolution's delete scoped by account alone cannot be told from the delete by intent: the intents table is keyed by account, so one account has one intent and no fixture can distinguish the two (the review-six row says the same of its surviving mutant); eight mutants, one per fix, were killed in a scratch copy at the time (the runs are not reproducible from this file); two items are left open and say so.

| --- | --- | --- | --- |

@@ -153,7 +153,7 @@ The fixes are in the commit that adds this section and have not been read by a r

## Review six (at b51c2a26; Opus accepted with fixes, Astra rejected)

-Reports: `OPUS-REVIEW-multi-currency-review6.md`, `CODEX-FINDINGS-multi-currency-review6.md`. Both found the fifth round's fixes present and the GBP purchase consistent end to end, and both found the same defect at the top: the quote signature was keyed by the session token the form discloses. Everything of either is fixed below with a test that fails without it; six mutants were killed in a scratch copy.

+Reports: `OPUS-REVIEW-multi-currency-review6.md`, `CODEX-FINDINGS-multi-currency-review6.md`. Both found the fifth round's fixes present and the GBP purchase consistent end to end, and both found the same defect at the top: the quote signature was keyed by the session token the form discloses. Everything of either is fixed below with a test that fails without it; five mutants were killed in a scratch copy and one survives by construction (the account-only delete, as the row below says).

| --- | --- | --- | --- |

@@ -228,7 +228,7 @@ Reports: `OPUS-REVIEW-multi-currency-review10.md`, `CODEX-FINDINGS-multi-currenc

| --- | --- | --- | --- |

| Opus P3-1, Astra 1 (P3): reconcile checked the bound session with `ownSession` and the currency, not `intentSession`, so a subscription event delivered before the completion event recorded a purchase whose session the binder would have reviewed for its plan or band. | Fixed: reconcile judges the intent's own, complete session with `intentSession` after ownership, and a mismatch files the same intent-keyed review the binder files and records nothing; the intent stands. The binder's inline comparison is `intentSession` too, so the purchase judgement has one definition. | `billingservice.go` `reconcileBillingSubscription`, `bindBillingCheckoutFor` | `TestASubscriptionEventBeforeTheCompletionMeetsTheSameJudgement` (the session's metadata alone altered, plan and band in turn, through the fake's new `sessionMeta` knob; `customer.subscription.created` first: one review keyed by the intent with the session as its reference, nothing recorded, the intent standing; then the completion and a second subscription event: still one) |

-| Astra 2 (P3): a session whose `client_reference_id` is A's and whose `metadata.account_sqid` is B's passed `ownSession` and `intentSession`, so the resume and listing paths could offer or keep what the binder would refuse. | Fixed: `ownSession` requires the account metadata to name the account too; such a session is nobody's on every path, and, being nobody's, is not expired either. | `billingservice.go` `ownSession` | `TestOwnSessionClauses` ("another account in the metadata"); `TestASessionNamingTwoAccountsIsNobodys` (a bound open session read back so is not offered, not expired, the intent left as it was; a listed one is not bound, and the same listing binds it once its accounts agree) |

+| Astra 2 (P3): a session whose `client_reference_id` is A's and whose `metadata.account_sqid` is B's passed `ownSession` and `intentSession`, so the resume and listing paths could offer or keep what the binder would refuse. | Fixed: `ownSession` requires the account metadata to name the account too; such a session is nobody's on every path, and, being nobody's, is not expired either. [Review eleven: for the session an intent is bound to, that left the intent wedged with no review; from the eleventh-round commit such a read files the intent-keyed review, see "Review eleven".] | `billingservice.go` `ownSession` | `TestOwnSessionClauses` ("another account in the metadata"); `TestASessionNamingTwoAccountsIsNobodys` (a bound open session read back so is not offered, not expired, the intent left as it was; a listed one is not bound, and the same listing binds it once its accounts agree) |

| Opus P3-2, Astra claims: the roll note promised a heal that review nine removed. | Corrected: the prohibition is absolute; a mark an older binary makes stays until an operator's door that does not exist. | this file, roll note | |

| Astra claims: the single-review reader `billingReview` did not carry `reference`. | Fixed: it selects the column; the resolution test reads the review singly. | `billingstore.go` `billingReview` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` |

| Astra claims: the post-resolution test replayed an open replacement session, not the retired session's late completion, because the fake had one session id. | Fixed: the second purchase binds `cs_test_second` (the fake answers any session path with its one session, so a console that asked for another id sees the mismatch), and the late webhook for `cs_test_paid` is answered, once, with the retired session as it was (complete, EUR, the retired intent's metadata): nothing recorded, reviewed or expired, the standing intent untouched. | `billingservice_test.go`, `billingcurrency_test.go` | the same test |

@@ -236,3 +236,18 @@ Reports: `OPUS-REVIEW-multi-currency-review10.md`, `CODEX-FINDINGS-multi-currenc

| Both: stale sentences in this file (review seven's agreement sentence; "every seventh-round finding closed"; the review-eight heal row; the terms-version row; "generated"/"uniformly distinguishing" in review four; the review-nine summary). | Corrected in place, each with the correction in brackets where the original sentence is history. | this file | |

Still open after this round, as both reviewers list: everything that needs Stripe test mode (the list in both reports and in `ENABLEMENT.md` section 6, now with session metadata immutability, line-item reads and webhook ordering added), the two unreviewed system strings, and the operator's door for a quantity-history gap. The eleventh review reads this commit.

+## Review eleven (reviewed `25487506`; Opus accepted with fixes, Astra accepted with fixes)

+Reports: `OPUS-REVIEW-multi-currency-review11.md`, `CODEX-FINDINGS-multi-currency-review11.md` (in the tree from this commit, with the tenth's since `28dc2629`). Both found every tenth-round finding closed, each with a test that fails without its fix, and nothing in a money path; both accept with fixes for the second round running. The fixes, each with a test that fails without it; five mutants killed in a scratch copy (the bound-session review never filed; the subscription mismatch's review never filed; the store's account-metadata check on an expiry's answer removed; and, surviving by construction, the two service-level copies of that check, removed in favour of the one in the store):

+| --- | --- | --- | --- |

+| Opus P3-1: the session an intent is bound to, read back as nobody's (its account metadata, client reference or customer changed at the provider), wedged the intent: every click and pass refused, nothing expired, no review, the account unable to buy or be deleted, and a payment through the URL the buyer held produced only a rejected-events row. | Fixed: `ownBoundSession`, used wherever the bound session is read (resume of the previous and the current intent, the pass, reconcile, the expected-intent binder), files the intent-keyed review with the session as its reference when the read is of the bound session (its id and the intent's id) and `ownSession` says no; the intent stands until the operator resolves the review, which retires it; the session is left as it is at the provider (the console acts on no session it cannot call its own; the review names it for the operator). A read that is not of the bound session (a listing, a create response) files nothing. The existing "bound session completed for another customer" case now expects that one review. | `billingservice.go` `ownBoundSession`, `billingpass.go` | `TestABoundSessionReadBackAsNobodysIsReviewed` (the read files one review naming the session; paid through the URL, a pass and both webhooks record nothing and add nothing; the resolution retires the intent and a new purchase opens); `TestOwnershipIsEnforcedOnEveryPath` |

+| Opus P3-2: a subscription that names the intent and is not its purchase (plan, band, terms, price terms or quantity changed at the provider between payment and recording) was refused with no review. | Fixed: once the intent is bound, reconcile files the same intent-keyed review with the bound session as reference, and records nothing; a subscription naming another intent is still a bare refusal. | `billingservice.go` `reconcileBillingSubscription` | `TestASubscriptionNotTheIntentsPurchaseIsReviewed` (the session unchanged through `sessionMeta`, the subscription's band and price another band's) |

+| Astra 1 (P3): an authorised expiry's answer that named another account in its metadata was accepted as proof and retired the intent. | Fixed in the store: `retireIntent` requires the answer's account metadata to name the account, beside the client reference, customer, intent, mode, status and payment it already required. The two service paths that ask for the expiry rely on that one check (two copies there were written and removed: the store's check made them indistinguishable). | `billingstore.go` `retireIntent` | `TestAnExpiryAnsweredForAnotherAccountRetiresNothing` (both paths: a changed currency, and a session read back in another currency; the intent and its session as they were, no replacement opened); `TestQuantityAuditExpiredCheckoutRequiresBoundProviderProof` (the new mutation) |

+| Astra claims: review four's qualification about the delete predicate; review six's mutant count; `intentSession`'s "every session" and the already-recorded fast path. | Corrected in place; the fast path is named in the comment. | this file, `billingservice.go` | |

+Still open, as both reviewers list: everything that needs Stripe test mode (both lists, with session metadata mutability and a subscription changed between payment and recording added this round), the two unreviewed system strings, the operator's door for a quantity-history gap, and a listing of rejected events for the operator (not built; the reviews above are what reaches the operator). The twelfth review reads this commit.

diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review11.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review11.md

new file mode 100644

index 000000000..4168b4a11

--- /dev/null

+++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review11.md

@@ -0,0 +1,123 @@

+# Eleventh review of `feat/carloku-multi-currency-20261001` at `25487506` (base `12e70d95`)

+I read the source with `git show` only, modified nothing and ran no tests. The tenth-round delta (`faf1bd25..25487506`) changes code in three places:

+- `ownSession` gains the account-metadata clause.

+- `reconcileBillingSubscription` now judges the bound session with `intentSession` and files the review the binder files.

+- `billingReview` now selects `reference`.

+The rest of the delta is tests, the test fake and the dispositions. `internal/plans`, `cmd`, `billingstripe`, `billingpage.go`, `billinghandlers.go`, `billingcurrency.go`, the trigger code and the quantity code are unchanged since `faf1bd25`. Sections A–C, E–K, M–Q, T–X and Z–DD therefore stand as both tenth reports traced them. I re-read only what the delta touches or interacts with.

+The tenth reports are not in the tree at `25487506`. They land in the next commit on the branch, `28dc2629`, and I read them from there.

+On the site, `3f1610d` changes only one test's name. Its `pricebook.json` is byte-identical to the platform's `public-pricebook.json` at `25487506` (sha256 `d91d5a48…`).

+## Tenth-review findings

+| Finding | Closed? | Would its test pass without its fix? |

+|---|---|---|

+| **Opus P3-1 / Astra 1 (P3)**: reconcile judged the bound session on ownership and currency only | **Closed.** `billingservice.go:805` runs `intentSession` after `ownSession` (`:798`). A mismatch files `fileCheckoutRefusedReview(intent, session.ID, now)`, records nothing and leaves the intent standing. The binder's inline comparison is now `intentSession` too (`:980`), so the judgement has one definition. | **No.** `TestASubscriptionEventBeforeTheCompletionMeetsTheSameJudgement` fails without the fix: the `sessionMeta` knob separates the session's metadata from the subscription's `f.meta`, and the old reconcile records the subscription, so `state.Subscription != ""`. |

+| **Opus P3-2**: the roll note promised a heal that no longer exists | **Closed.** The note now says the prohibition is absolute and the mark stays until an operator door exists. | Prose; no test. |

+| **Astra 2 (P3)**: a session naming account A in `client_reference_id` and B in `metadata.account_sqid` passed `ownSession` | **Closed.** `:265` requires both. | **No.** Both subtests of `TestASessionNamingTwoAccountsIsNobodys` fail without the clause: the old code offers the URL on resume and binds the listed session. The `TestOwnSessionClauses` case fails too. |

+| **Opus claims** (roll note; the `intentSession` comment and design line 152; the terms-version row; "six stale sentences"; line 194; the heal rows at 199 and 204) | **Closed.** The comment and design are now true of the code. Every other item is corrected in place, with the history in brackets. | n/a |

+| **Astra claims** (review seven's agreement sentence; line 194; the ninth summary; the single-review `reference`; review four's "uniformly"; the late-completion replay; the site test's name) | **Closed.** `billingReview` selects `reference` (`billingstore.go:786`), and the resolution test reads it singly; that read fails without the column. Review four is qualified, and the replay is rebuilt (see GG). The site test is renamed at `3f1610d`. | The `reference` read: no. The replay: see GG. |

+**Severity.** The two reviewers did not disagree this round: both rated the shared finding P3 and both accepted with fixes. Astra's finding 2, which Opus did not report, is P3 from the code. It needs session metadata changed at the provider after creation, and no buyer-controlled path does that.

+## Findings

+### P3-1. A bound or listed session the console cannot call its own wedges the intent, files no review, and can still be paid

+- **Where:**

+ - `billingservice.go:264-272` (`ownSession`).

+ - Resume returns `errBillingIdentity` at `:567` and `:620`.

+ - The pass returns `errBillingIdentity` at `billingpass.go:195-197`, ahead of its `expired` branch.

+ - The listing returns at `billingservice.go:1216-1218`.

+ - Reconcile returns at `:798`.

+ - The webhook binder returns at `:957`.

+- **Scenario:**

+ 1. An account's GBP intent is bound to `cs_X`, which the console created under `carloku-checkout-<intent>` and judged its own.

+ 2. Someone with dashboard or API access at Stripe sets `cs_X`'s `metadata.account_sqid` to another account's sqid (or changes its `client_reference_id`, or its customer).

+ 3. From then on:

+ - Every click returns `errBillingIdentity` and offers nothing.

+ - Every pass returns `errBillingIdentity`. It never reaches the `expired` branch, so the intent is never retired, even after Stripe expires the session 24 hours later.

+ - The account can never buy again.

+ - `resolveIntentForDeletion` (`:681`) goes through `reconcilePendingIntent` and fails, so account deletion is blocked.

+ 4. The session was not expired. A buyer who still has its URL can pay it within its lifetime. Then:

+ - The completion webhook is rejected at `:957`.

+ - The subscription webhook is rejected at `:798`.

+ - Both land only in `billing_rejected_events` (`acknowledgeBillingRejection`), which no operator API lists.

+ - No review is filed, so nothing reaches the operator. Money is taken and nothing is recorded.

+ 5. The same wedge follows from an unbound intent whose listed session (`:1216`) or create response (`:662`) is nobody's. There is no money risk there, because the URL was never disclosed.

+- **Is not expiring the right call (FF)?**

+ - It is right for an id the provider *hands back*: a refused create response, or a listing.

+ - For the id the console itself bound (`intent.SessionID`), not expiring is the wrong call. Stripe session ids are unique, and the console created and judged this one. A contradiction read back later is a provider-side change that a person must see.

+ - The class predates round ten (a changed `client_reference_id` or customer did the same). Round ten widened it to the account metadata and recorded "not expired either" as intended.

+- **Smallest acceptable fix:** where `session.ID == intent.SessionID` and `session.Metadata["checkout_intent"] == intent.ID` but `ownSession` fails:

+ - File the intent-keyed `billing_checkout_currency_refused` review with the session as its reference. The review is how an operator retires the intent.

+ - Return `errBillingIdentity`.

+ - Preferably also expire the session if it is open; at least record that it is left open.

+ - Test: the bound-session case from `TestASessionNamingTwoAccountsIsNobodys`, then the session completed, then a pass and both webhooks. Expect one review and nothing recorded. Resolving the review retires the intent and a new purchase opens.

+### P3-2. A purchase whose *subscription* differs from the intent is refused with no review (pre-existing; the "none" half of EE)

+- **Where:** `billingservice.go:767-768` (and the price, currency and identity checks above it at `:733-756`). They return `errBillingIdentity` before the session is read and before any review.

+- **Scenario:**

+ 1. Checkout completes and the session matches the intent, so the binder binds it (`:996`).

+ 2. Before the console records anything, someone at Stripe changes the subscription's price, or its metadata plan or band with a matching price.

+ 3. Reconcile refuses on every path. The only record is a `billing_rejected_events` row.

+ 4. The customer is charged; no review is filed; the intent stands, and the pass retries forever.

+- **Severity:** a narrower variant of P3-1. It needs a provider-side change between payment and recording, and the money charged is what Stripe says, never a conversion.

+- **Smallest acceptable fix:** in reconcile's first-purchase branch, when `sub.Metadata["checkout_intent"] == intent.ID` and the intent is bound to an owned, completed session, file the same intent-keyed review on any intent and subscription mismatch, in place of a bare refusal.

+### P3-3. Test fake: the expire handler answers every id as `cs_test_paid`

+- **Where:** `billingservice_test.go:261`. Round ten widened the route to any id (`:394`) but kept the hard-coded id in the answer.

+- **Scenario:** a test that expires `cs_test_second` (or any non-default `sessionID`) gets back `cs_test_paid`. `refuseSession` (`:308`) and the resume path (`:577`) then see a mismatch and stay pending. A test of expiring a second session would fail for the fake's reason, or pass for the wrong one. None does today.

+- **Fix:** answer with the requested id, or with `sessionID` when it is set.

+## The added sections

+- **EE.** Reconcile and the binder now apply the same predicate under the same billing lock and file the same intent-keyed review. `fileCheckoutRefusedReview` opens a review only while the intent stands, and the review's key is unique, so subscription-first, completion-first or both, from the pass or the webhook, give exactly one review. The two paths do not both file and file more than one. They can file none in the provider-side cases P3-1 and P3-2.

+ Reconcile files before checking `payment_status`, as the binder files on `Status == "complete"` alone, so the two are consistent. A completed, unpaid (async) session that mismatches is reviewed on both paths.

+- **FF.** The account-metadata clause holds on every path:

+ - resume of the previous intent (`:561`) and of the current intent (`:616`);

+ - fresh create and its refusal (`:651`, `:662`);

+ - interactive replay (`:1181`, `:1189`);

+ - the listing (`:1216`);

+ - the pass (`billingpass.go:195`);

+ - the expected-intent binder (`:948`, where the extra `account_sqid` comparison is now redundant);

+ - the webhook binder (`:957`, `:977`);

+ - reconcile (`:798`).

+ No session naming two accounts is offered, bound, recorded or expired. The only thing still acted on without the clause is `ExpireCheckout`'s *response* in `refuseSession` and resume. `retireIntent` re-checks its mode, `checkout_intent`, client reference and customer, but not `account_sqid`. That is harmless: it retires the console's own intent on proof of an unpaid expiry. What stays open at the provider is the case in P3-1.

+- **GG.** The test now establishes its claim:

+ - The second purchase binds `cs_test_second`.

+ - The late `checkout.session.completed` for `cs_test_paid` is answered once (`sessionAnswers`) with the retired session as it was: complete, EUR, the old intent's metadata, copied before the second checkout overwrote `f.meta`.

+ - The binder refuses it at `ownSession`, because `checkout_intent` and the bound id are both the second intent's.

+ - If the retired completion were recorded, `state.Subscription != ""` would fail. A refiling would also find no standing intent.

+ The test does not assert the webhook's status (it is 204, through the rejection record).

+- **S / V.** `billing_checkout_intents` is keyed by `account_id` (`billingstore.go:124`), so no other intent can exist while the refused one stands. A repeated resolution returns before its `DELETE`. The `id=?` predicate therefore cannot touch another intent, and no test could distinguish it, as the dispositions now say. The retirement is recorded in `billing_retired_intents` with the session id.

+- **L / R / W / Y / AA / DD.** Every path checks ownership before acting, and then `intentSession` before offering, binding or recording. No resume or listing path keeps a session that differs from its intent in plan, band, terms, book or currency.

+ A session's line items and discount are still never compared. Reconcile checks the subscription's price and quantity instead.

+- **CC / X / N / T / E / H / M / Q / U / Z / BB / O / B / C / A / F / G / I / J / K / P.** Unchanged since `faf1bd25`. Both tenth reports' traces stand, including the GBP purchase end to end with no conversion path, the trigger's span rule, and the site's links, gate and exactness check. I did not re-trace them this round.

+## Claims the code contradicts

+1. The review-ten row for Astra 2 says such a session "is nobody's on every path, and, being nobody's, is not expired either". That is accurate, but it omits that the intent is then never retired, the account can neither buy nor be deleted, and a payment through a URL already held produces no review (P3-1). The design's sentence "every session the console … reads back or binds passes one check" says nothing of this outcome.

+2. The comment at `billingservice_test.go:403-406` says "a console that asked for another id sees the mismatch". That holds for reads only. The expire answer always says `cs_test_paid` (P3-3).

+3. "Three mutants were killed in a scratch copy" (review ten). This is consistent with my source counterfactuals, but the runs cannot be reproduced from the file.

+## Verify in Stripe test mode before enabling

+Both tenth reports' lists stand: catalog and paging; the currency × country matrix including IN with GBP; the refusal shape; metadata immutability; line items; overages per currency; proration and renewal; lost-response recovery; refusal and resolution with late delivery; customer currency locks and discounts; the rollback and restart rehearsal.

+Added this round:

+1. **Session metadata.** Update a Checkout Session's metadata (account or intent) after creation, through the API and the dashboard. This shows whether P3-1's precondition can occur, and what the console does with it: today, a wedged intent and no review.

+2. **Subscription changed between payment and recording.** Change the subscription's price or metadata between `checkout.session.completed` and the console's reconcile (hold the webhook). This shows P3-2: a refusal with no review.

+**Verdict: accept with fixes.** Every tenth-round finding is closed, each with a test that fails without its fix. Nothing touches money. Before enabling, give a bound session that reads back as nobody's an intent-keyed review instead of a silent wedge (P3-1), and correct the test fake's expire answer (P3-3). P3-2 can be dispositioned.

diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go

index 2a85a2d46..58967f852 100644

--- a/internal/console/billingcurrency_test.go

+++ b/internal/console/billingcurrency_test.go

@@ -2696,9 +2696,11 @@ func TestOwnSessionClauses(t *testing.T) {

// Ownership is enforced on every path the provider's answer reaches: a

// listed session with the intent's id but another customer is neither

-// expired nor reviewed; a read of the bound session answered with another

-// session is neither bound, recorded nor reviewed, through the pass, the

-// webhook, a direct binding and reconcile.

+// expired nor reviewed; the bound session read back as another customer's

+// is neither bound, recorded nor expired, through the pass, the webhook, a

+// direct binding and reconcile, and is reviewed once (it is the session

+// the console itself bound); a read answered with another account's

+// everything is acted on for nobody.

func TestOwnershipIsEnforcedOnEveryPath(t *testing.T) {

reviews := func(t *testing.T, fx *consoleFixture, acct *Account) int {

var n int

@@ -2758,9 +2760,18 @@ func TestOwnershipIsEnforcedOnEveryPath(t *testing.T) {

}

_ = fx.srv.billingPass(context.Background(), f.now.Add(time.Minute))

webhook(t, fx, "evt_customer_"+currency, http.StatusNoContent)

+ // The bound session, read back as another customer's, is a

+ // contradiction at the provider: nothing is recorded or

+ // expired from it, and the one intent-keyed review names it

+ // for the operator (review eleven; before it, nothing was

+ // filed and the intent was wedged in silence).

state, _ := fx.st.billingAccount(acct.ID)

- if state.Subscription != "" || f.expireCalls != 0 || reviews(t, fx, acct) != 0 {

- t.Fatalf("currency %q: state %+v, %d expiries, %d reviews; want nothing recorded, expired or reviewed for another customer's session", currency, state, f.expireCalls, reviews(t, fx, acct))

+ if state.Subscription != "" || f.expireCalls != 0 || reviews(t, fx, acct) != 1 {

+ t.Fatalf("currency %q: state %+v, %d expiries, %d reviews; want nothing recorded or expired, and one review of the bound session", currency, state, f.expireCalls, reviews(t, fx, acct))

+ }

+ var reference string

+ if err := fx.st.db.QueryRow(`SELECT reference FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=?`, acct.ID, intent.ID, billingCheckoutRefusedReason).Scan(&reference); err != nil || reference != intent.SessionID {

+ t.Fatalf("currency %q: the review's reference %q %v; want the bound session", currency, reference, err)

}

})

@@ -3307,3 +3318,182 @@ func TestAGapFromAnObsoleteOutcomeListStaysHeld(t *testing.T) {

t.Fatalf("the account with a kept gap is not held (%v %v)", held, err)

}

+// 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

+// intent with no review, and could still be paid through a URL held.)

+func TestABoundSessionReadBackAsNobodysIsReviewed(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)

+ }

+ review := func() (open int, occurrences int64, reference string) {

+ 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)

+ if errors.Is(err, sql.ErrNoRows) {

+ return 0, 0, ""

+ }

+ if err != nil {

+ t.Fatal(err)

+ }

+ return 1, occurrences, reference

+ }

+ f.mu.Lock()

+ f.meta["account_sqid"] = "zzz"

+ f.mu.Unlock()

+ if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now.Add(time.Minute)); dest != "" || !errors.Is(err, errBillingIdentity) {

+ t.Fatalf("the bound session named another account: dest %q err %v", dest, err)

+ }

+ if open, n, ref := review(); open != 1 || n != 1 || ref != intent.SessionID {

+ t.Fatalf("after the read: %d open, %d occurrences, reference %q; want one review naming the session", open, n, ref)

+ }

+ if f.expireCalls != 0 || f.sessionStatus != "open" {

+ t.Fatalf("%d expiries, session %s; want the session left as it is", f.expireCalls, f.sessionStatus)

+ }

+ // Paid through the URL the buyer held: the pass and both webhooks

+ // record nothing and add nothing to the one review.

+ f.mu.Lock()

+ f.sessionStatus, f.subscriptionPrice = "complete", intent.Price

+ f.mu.Unlock()

+ _ = fx.srv.billingPass(context.Background(), f.now.Add(2*time.Minute))

+ for _, ev := range [][2]string{{"evt_nobody_done", "checkout.session.completed"}, {"evt_nobody_sub", "customer.subscription.created"}} {

+ object := intent.SessionID

+ if ev[1] != "checkout.session.completed" {

+ object = "sub_paid"

+ }

+ if code := f.webhookPost(t, fx, ev[0], ev[1], object); code != http.StatusNoContent {

+ t.Fatalf("%s: %d", ev[1], code)

+ }

+ if open, n, _ := review(); open != 1 || n != 1 {

+ t.Fatalf("after a pass and both webhooks: %d open, %d occurrences; want the one review", open, n)

+ }

+ if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "" || f.expireCalls != 0 {

+ t.Fatalf("state %+v, %d expiries; want nothing recorded or expired", state, f.expireCalls)

+ }

+ if standing, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || standing.ID != intent.ID {

+ t.Fatalf("intent %+v %v; want it standing", standing, err)

+ }

+ // The operator resolves the review: the intent is retired and a new

+ // purchase opens.

+ 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: "refunded"}, f.now.Add(10*time.Minute)); err != nil {

+ t.Fatalf("resolve: %v", err)

+ }

+ if _, err := fx.st.billingCheckoutIntent(acct.ID); err == nil {

+ t.Fatal("the intent still stands after its review was resolved")

+ }

+ f.mu.Lock()

+ f.sessionStatus, f.sessionID, f.subscriptionPrice = "open", "cs_test_second", ""

+ f.mu.Unlock()

+ if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now.Add(20*time.Minute)); err != nil || dest == "" {

+ t.Fatalf("a new purchase after the resolution: dest %q err %v", dest, err)

+ }

+ if second, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || second.ID == intent.ID || second.SessionID != "cs_test_second" {

+ t.Fatalf("the new intent: %+v %v", second, err)

+ }

+// A subscription that names the intent and is not the purchase it

+// describes (its band changed at the provider between the payment and the

+// recording, with a matching price) is a review, not a bare refusal, once

+// the intent is bound. (Review eleven, Opus P3-2.)

+func TestASubscriptionNotTheIntentsPurchaseIsReviewed(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 == "" || intent.Band == plans.BandD {

+ t.Fatalf("fixture: intent %+v %v", intent, err)

+ }

+ // The session is the intent's purchase exactly; the subscription's

+ // band and price are another band's.

+ sessionMeta := map[string]string{}

+ f.mu.Lock()

+ for k, v := range f.meta {

+ sessionMeta[k] = v

+ }

+ f.sessionMeta = sessionMeta

+ f.meta["band"] = plans.BandD

+ f.sessionStatus, f.subscriptionPrice = "complete", currencyPriceID(plans.PlanPersonal, plans.BandD, plans.GBP)

+ f.mu.Unlock()

+ if code := f.webhookPost(t, fx, "evt_sub_band", "customer.subscription.created", "sub_paid"); code != http.StatusNoContent {

+ t.Fatalf("the subscription event: %d", code)

+ }

+ 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 keyed by the intent naming its session", occurrences, reference, err)

+ }

+ if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "" {

+ t.Fatalf("the subscription was recorded: %+v", state)

+ }

+ if standing, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || standing.ID != intent.ID {

+ t.Fatalf("intent %+v %v; want it standing", standing, err)

+ }

+// An authorised expiry whose answer names another account retires nothing:

+// the answer is judged as the read was, on both paths that expire a bound

+// session (a changed purchase, and a session read back as not the

+// purchase). (Review eleven, Astra 1.)

+func TestAnExpiryAnsweredForAnotherAccountRetiresNothing(t *testing.T) {

+ for name, provoke := range map[string]func(fx *consoleFixture, f *fakeBillingStripe, acct *Account) error{

+ "the buyer changes currency": func(fx *consoleFixture, f *fakeBillingStripe, acct *Account) error {

+ _, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "usd", f.now.Add(time.Minute))

+ return err

+ },

+ "the session reads back in another currency": func(fx *consoleFixture, f *fakeBillingStripe, acct *Account) error {

+ f.mu.Lock()

+ f.sessionCurrency = "eur"

+ f.mu.Unlock()

+ _, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now.Add(time.Minute))

+ return err

+ },

+ } {

+ t.Run(name, func(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)

+ }

+ before, err := fx.st.billingCheckoutIntent(acct.ID)

+ if err != nil || before.SessionID == "" {

+ t.Fatalf("fixture: intent %+v %v", before, err)

+ }

+ expireMeta := map[string]string{}

+ f.mu.Lock()

+ for k, v := range f.meta {

+ expireMeta[k] = v

+ }

+ expireMeta["account_sqid"] = "zzz"

+ f.expireMeta = expireMeta

+ f.mu.Unlock()

+ if err := provoke(fx, f, acct); !errors.Is(err, errBillingIdentity) {

+ t.Fatalf("%v; want an identity refusal of the expiry's answer", err)

+ }

+ if f.expireCalls != 1 {

+ t.Fatalf("%d expiries; want the one the console asked for", f.expireCalls)

+ }

+ after, err := fx.st.billingCheckoutIntent(acct.ID)

+ if err != nil || after.ID != before.ID || after.SessionID != before.SessionID {

+ t.Fatalf("intent %+v %v; want it as it was: an answer naming another account retires nothing", after, err)

+ }

+ if f.checkoutCalls != 1 {

+ t.Fatalf("%d sessions created; want no replacement opened on that answer", f.checkoutCalls)

+ }

+ })

+ }

diff --git a/internal/console/billingmembership_test.go b/internal/console/billingmembership_test.go

index d23be7081..241cddf28 100644

--- a/internal/console/billingmembership_test.go

+++ b/internal/console/billingmembership_test.go

@@ -323,7 +323,7 @@ func TestMembershipLivePersonalCheckoutLocksGrowthUntilResolved(t *testing.T) {

t.Fatal(err)

}

intent.SessionID = "cs_live"

- if err := f.s.expireBillingCheckout(intent, &billingstripe.Session{ID: intent.SessionID, Status: "expired", LiveMode: true, ClientReferenceID: f.sqid, Metadata: map[string]string{"checkout_intent": intent.ID}}, f.now); err != nil {

+ if err := f.s.expireBillingCheckout(intent, &billingstripe.Session{ID: intent.SessionID, Status: "expired", LiveMode: true, ClientReferenceID: f.sqid, Metadata: map[string]string{"checkout_intent": intent.ID, "account_sqid": f.sqid}}, f.now); err != nil {

t.Fatal(err)

}

if _, err := f.s.AcceptInvite(tok, "a@example.test", f.now); err != nil {

diff --git a/internal/console/billingpass.go b/internal/console/billingpass.go

index d60de3f64..658d38dab 100644

--- a/internal/console/billingpass.go

+++ b/internal/console/billingpass.go

@@ -192,7 +192,9 @@ func (s *Server) reconcilePendingIntent(ctx context.Context, intent billingInten

if err != nil || owner == nil {

return err

}

- if !b.ownSession(session, intent, owner) {

+ if own, err := s.ownBoundSession(session, intent, owner, now); err != nil {

+ return err

+ } else if !own {

return errBillingIdentity

}

switch session.Status {

@@ -214,7 +216,7 @@ func (s *Server) reconcilePendingIntent(ctx context.Context, intent billingInten

return err

}

if session.ID == intent.SessionID && !b.intentSession(session, intent, acct) && b.ownSession(session, intent, acct) {

- if err := s.refuseSession(ctx, intent, session.ID, session, now); err != nil && !errors.Is(err, errBillingIdentity) {

+ if err := s.refuseSession(ctx, intent, session.ID, session, owner, now); err != nil && !errors.Is(err, errBillingIdentity) {

return err

}

diff --git a/internal/console/billingquantityaudit_test.go b/internal/console/billingquantityaudit_test.go

index fb07bb9ab..15894180b 100644

--- a/internal/console/billingquantityaudit_test.go

+++ b/internal/console/billingquantityaudit_test.go

@@ -130,13 +130,18 @@ func TestQuantityAuditExpiredCheckoutRequiresBoundProviderProof(t *testing.T) {

if err := f.s.AddMember(f.id, "added@example.test", RoleMember, f.now); err != nil {

t.Fatal(err)

}

- proof := billingstripe.Session{ID: intent.SessionID, Status: "expired", LiveMode: true, ClientReferenceID: f.sqid, Metadata: map[string]string{"checkout_intent": intent.ID}}

+ proof := billingstripe.Session{ID: intent.SessionID, Status: "expired", LiveMode: true, ClientReferenceID: f.sqid, Metadata: map[string]string{"checkout_intent": intent.ID, "account_sqid": f.sqid}}

for _, mutate := range []func(*billingstripe.Session){

func(s *billingstripe.Session) { s.ID = "cs_other" },

func(s *billingstripe.Session) { s.Status = "open" },

func(s *billingstripe.Session) { s.LiveMode = false },

func(s *billingstripe.Session) { s.ClientReferenceID = "other" },

func(s *billingstripe.Session) { s.Metadata = nil },

+ // The answer's own account metadata names another account (review

+ // eleven, Astra 1): nobody's, retires nothing.

+ func(s *billingstripe.Session) {

+ s.Metadata = map[string]string{"checkout_intent": intent.ID, "account_sqid": "other"}

+ },

func(s *billingstripe.Session) { s.SubscriptionID = "sub_paid" },

func(s *billingstripe.Session) { s.PaymentStatus = "paid" },

} {

diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go

index f4fecd799..7d7552258 100644

--- a/internal/console/billingservice.go

+++ b/internal/console/billingservice.go

@@ -239,7 +239,9 @@ func (s *Server) checkoutParams(acct *Account, intent billingIntent) billingstri

// to a buyer or records a purchase from passes through here, however it

// was come by: created, replayed, listed, or read back by its id; and

// every session the console expires, reviews, binds or records from

-// passes ownSession first.

+// passes ownSession first. The one read that passes neither is the

+// webhook binder's fast path for a subscription already recorded, which

+// returns the recorded id and records nothing.

func (b *billingRuntime) intentSession(session *billingstripe.Session, intent billingIntent, acct *Account) bool {

if !b.ownSession(session, intent, acct) {

return false

@@ -271,6 +273,31 @@ func (b *billingRuntime) ownSession(session *billingstripe.Session, intent billi

return intent.SessionID == "" || intent.SessionID == session.ID

}

+// ownBoundSession is ownSession for a read of the session an intent is

+// bound to, with one more thing when the answer is no: the console created

+// that session and judged it its own when it bound it, so a read of it

+// that names another account, customer or mode is a contradiction at the

+// provider that a person must see. The intent-keyed review is filed with

+// the session as its reference (the same review a mismatched purchase

+// files); the intent stands, so no second purchase opens beside a session

+// that may still be paid, until the operator resolves the review, which

+// retires it; and the session is left as it is at the provider, since the

+// console acts on no session it cannot call its own (the review names it

+// for the operator to expire by hand). A read that is not of the bound

+// session (a listing, a create response) files nothing: nothing was ever

+// disclosed on it.

+func (s *Server) ownBoundSession(session *billingstripe.Session, intent billingIntent, acct *Account, now time.Time) (bool, error) {

+ if s.billing.ownSession(session, intent, acct) {

+ return true, nil

+ }

+ if session != nil && intent.SessionID != "" && session.ID == intent.SessionID && session.Metadata["checkout_intent"] == intent.ID {

+ if err := s.store.fileCheckoutRefusedReview(intent, session.ID, now); err != nil {

+ return false, err

+ }

+ return false, nil

// refuseSession deals with a session that is this intent's and is not the

// purchase the intent describes: in another currency, converted, or with

// Adaptive Pricing on it. Such a session is never offered to the buyer and

@@ -294,7 +321,7 @@ func (b *billingRuntime) ownSession(session *billingstripe.Session, intent billi

// session is the session as last read, or as the client refused it when

// it was created. The caller has established that it is this intent's

// own (ownSession): nothing here expires a session on its id alone.

-func (s *Server) refuseSession(ctx context.Context, intent billingIntent, sessionID string, session *billingstripe.Session, now time.Time) error {

+func (s *Server) refuseSession(ctx context.Context, intent billingIntent, sessionID string, session *billingstripe.Session, acct *Account, now time.Time) error {

if session != nil && (session.Status == "complete" || session.PaymentStatus == "paid") {

if err := s.store.fileCheckoutRefusedReview(intent, session.ID, now); err != nil {

return err

@@ -308,6 +335,10 @@ func (s *Server) refuseSession(ctx context.Context, intent billingIntent, sessio

if expired.ID != sessionID || expired.Status != "expired" {

return errBillingIdentity

}

+ // The expiry's answer is judged as the read was, in the store

+ // (retireIntent): the request was the console's to make, and its

+ // answer is checked, not trusted. One that names another account,

+ // customer or intent is nobody's and retires nothing.

if intent.SessionID != "" {

err = s.store.expireBillingCheckout(intent, expired, now)

} else {

@@ -558,13 +589,15 @@ func (s *Server) startBillingCheckoutQuoted(ctx context.Context, acct *Account,

if session.ID != previous.SessionID {

return "", errBillingIdentity

}

+ if own, err := s.ownBoundSession(session, previous, acct, now); err != nil {

+ return "", err

+ } else if !own {

+ return "", errBillingIdentity

+ }

if !b.intentSession(session, previous, acct) {

// Bound to this intent and no longer the purchase it describes:

// a buyer who kept its URL must not be able to pay through it.

- if b.ownSession(session, previous, acct) {

- return "", s.refuseSession(ctx, previous, session.ID, session, now)

- }

- return "", errBillingIdentity

+ return "", s.refuseSession(ctx, previous, session.ID, session, acct, now)

}

if session.Status == "open" && (previous.Currency != request.Currency || previous.PriceBook != request.PriceBook || previous.Plan != request.Plan || previous.Band != request.Band || previous.Quantity != request.Quantity || previous.Country != request.Country || previous.Price != request.Price || previous.Coupon != request.Coupon || previous.LocDigest != request.LocDigest || previous.LiveMode != request.LiveMode || previous.Members != request.Members) {

// A bound parameter changed — including the currency, a corrected postal code,

@@ -613,12 +646,14 @@ func (s *Server) startBillingCheckoutQuoted(ctx context.Context, acct *Account,

if session.ID != intent.SessionID {

return "", errBillingIdentity

}

- if !b.intentSession(session, intent, acct) {

- if b.ownSession(session, intent, acct) {

- return "", s.refuseSession(ctx, intent, session.ID, session, now)

- }

+ if own, err := s.ownBoundSession(session, intent, acct, now); err != nil {

+ return "", err

+ } else if !own {

return "", errBillingIdentity

}

+ if !b.intentSession(session, intent, acct) {

+ return "", s.refuseSession(ctx, intent, session.ID, session, acct, now)

+ }

switch session.Status {

case "open":

return intent.URL, nil

@@ -651,7 +686,7 @@ func (s *Server) startBillingCheckoutQuoted(ctx context.Context, acct *Account,

if !b.ownSession(refusedSession, intent, acct) {

return "", errBillingUnavailable

}

- return "", s.refuseSession(ctx, intent, refusedSession.ID, refusedSession, now)

+ return "", s.refuseSession(ctx, intent, refusedSession.ID, refusedSession, acct, now)

}

if err != nil {

return "", err

@@ -661,7 +696,7 @@ func (s *Server) startBillingCheckoutQuoted(ctx context.Context, acct *Account,

}

if !b.intentSession(session, intent, acct) {

if b.ownSession(session, intent, acct) {

- return "", s.refuseSession(ctx, intent, session.ID, session, now)

+ return "", s.refuseSession(ctx, intent, session.ID, session, acct, now)

}

return "", errBillingUnavailable

}

@@ -771,7 +806,24 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s

if err != nil {

return err

}

- if intent.ID != sub.Metadata["checkout_intent"] || intent.Plan != plan || intent.Terms != terms || intent.Band != band || intent.Quantity != sub.Items[0].Quantity || intent.priceTerms() != priceTerms {

+ if intent.ID != sub.Metadata["checkout_intent"] {

+ return errBillingIdentity

+ }

+ if intent.Plan != plan || intent.Terms != terms || intent.Band != band || intent.Quantity != sub.Items[0].Quantity || intent.priceTerms() != priceTerms {

+ // A subscription that names this intent and is not the purchase

+ // it describes (its plan, band, terms, price terms or quantity

+ // changed at the provider after the purchase): the money is

+ // what the provider says and never a conversion, and nothing is

+ // recorded from it. Once the intent is bound (its session was

+ // the intent's own when it was bound) a person is told through

+ // the same intent-keyed review a mismatched session files, with

+ // that session as its reference; unbound, the completion event

+ // binds or reviews the session first.

+ if intent.SessionID != "" {

+ if err := s.store.fileCheckoutRefusedReview(intent, intent.SessionID, now); err != nil {

+ return err

+ }

return errBillingIdentity

}

if intent.SessionID == "" {

@@ -792,7 +844,9 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s

// The session read back must be the intent's own (its id, this

// account, the intent's customer, this mode), and must be the

// session of this subscription and its customer.

- if !b.ownSession(session, intent, acct) || session.SubscriptionID != id || session.CustomerID != sub.CustomerID {

+ if own, err := s.ownBoundSession(session, intent, acct, now); err != nil {

+ return err

+ } else if !own || session.SubscriptionID != id || session.CustomerID != sub.CustomerID {

return errBillingIdentity

}

// And the purchase the intent describes: plan, band, terms, price

@@ -945,7 +999,12 @@ func (s *Server) bindBillingCheckoutFor(ctx context.Context, sessionID string, e

if err != nil {

return "", err

}

- if owner == nil || !b.ownSession(session, *expected, owner) || session.Metadata["account_sqid"] != owner.Sqid {

+ if owner == nil {

+ return "", errBillingIdentity

+ }

+ if own, err := s.ownBoundSession(session, *expected, owner, time.Now()); err != nil {

+ return "", err

+ } else if !own {

return "", errBillingIdentity

}

@@ -1183,12 +1242,12 @@ func (s *Server) recoverReadyIntentSession(ctx context.Context, acct *Account, i

if !b.ownSession(refusedSession, intent, acct) {

return nil, errBillingIdentity

}

- return nil, s.refuseSession(ctx, intent, refusedSession.ID, refusedSession, now)

+ return nil, s.refuseSession(ctx, intent, refusedSession.ID, refusedSession, acct, now)

}

if err == nil {

if !b.intentSession(session, intent, acct) {

if b.ownSession(session, intent, acct) {

- return nil, s.refuseSession(ctx, intent, session.ID, session, now)

+ return nil, s.refuseSession(ctx, intent, session.ID, session, acct, now)

}

return nil, errBillingIdentity

}

@@ -1222,7 +1281,7 @@ func (s *Server) recoverReadyIntentSession(ctx context.Context, acct *Account, i

// describes. One that is this intent's and is not that purchase

// is expired and the intent retired.

if !b.intentSession(&session, intent, acct) {

- return nil, s.refuseSession(ctx, intent, session.ID, &session, now)

+ return nil, s.refuseSession(ctx, intent, session.ID, &session, acct, now)

}

return &session, nil

}

diff --git a/internal/console/billingservice_test.go b/internal/console/billingservice_test.go

index 48de1e2b4..820e9d617 100644

--- a/internal/console/billingservice_test.go

+++ b/internal/console/billingservice_test.go

@@ -55,6 +55,10 @@ type fakeBillingStripe struct {

// answering with session metadata that differs from the subscription's.

sessionSqid string

sessionMeta map[string]string

+ // expireMeta, when set, is the metadata the expiry's answer carries in

+ // place of meta: the provider answering an authorised expiry with a

+ // session that names another account.

+ expireMeta map[string]string

// 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.

@@ -258,7 +262,14 @@ func (f *fakeBillingStripe) serve(w http.ResponseWriter, r *http.Request) {

return

}

f.sessionStatus = "expired"

- out = map[string]any{"id": "cs_test_paid", "url": "https://checkout.stripe.com/c/paid", "status": "expired", "customer": "cus_paid", "livemode": false, "client_reference_id": f.sqid, "metadata": f.meta}

+ // The answer names the session that was asked for (as the provider

+ // does), with the metadata the fake was told to answer with.

+ expiredID := strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/v1/checkout/sessions/"), "/expire")

+ expiredMeta := f.meta

+ if f.expireMeta != nil {

+ expiredMeta = f.expireMeta

+ }

+ out = map[string]any{"id": expiredID, "url": "https://checkout.stripe.com/c/paid", "status": "expired", "customer": "cus_paid", "livemode": false, "client_reference_id": f.sqid, "metadata": expiredMeta}

case strings.HasPrefix(r.URL.Path, "/v1/checkout/sessions/") && r.Method == http.MethodGet:

// Any id: the session rendered is the fake's one session, whose id

// is sessionID or cs_test_paid; a console that asked for another

diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go

index c2e61fc1e..3044deaca 100644

--- a/internal/console/billingstore.go

+++ b/internal/console/billingstore.go

@@ -462,7 +462,7 @@ func (s *Store) retireIntent(intent billingIntent, proof *billingstripe.Session,

if err := tx.QueryRow(`SELECT sqid FROM accounts WHERE id=?`, intent.AccountID).Scan(&sqid); err != nil {

return err

}

- if proof.ClientReferenceID != sqid || (intent.Customer != "" && proof.CustomerID != intent.Customer) {

+ if proof.ClientReferenceID != sqid || proof.Metadata["account_sqid"] != sqid || (intent.Customer != "" && proof.CustomerID != intent.Customer) {

return errBillingIdentity

}

if _, err := tx.Exec(`UPDATE billing_quantity_events SET processed_at=?,processing_outcome='checkout_expired',processing_reference=? WHERE account_id=? AND intent_id=? AND subscription='' AND processed_at=0`, now.Unix(), proof.ID, intent.AccountID, intent.ID); err != nil {