amadan
Multi-currency: the seventh review's findings
One definition of the outcomes a processed quantity event may carry, used by the processing guard, the startup sweep and the obligation check, so a resolved refusal no longer holds the account with nothing open to clear it; the test follows the resolution through the obligation, a month's freeze and a second start. A caller working on a known intent binds through a door that judges the session it reads against that intent, by that intent's account, before any account is derived from the response. A refused session found by the listing is recorded by its id on the review and in the retirement record. The quoted country's band at render time is in the quote signature. The comments and the dispositions say what the code does.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
12 files changed,
+505
−36
docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md+1 −1docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review7.md+130 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+22 −4docs/superpowers/specs/OPUS-REVIEW-multi-currency-review7.md+130 −0internal/console/billingcurrency_test.go+125 −3internal/console/billinghandlers.go+9 −1internal/console/billingpage.go+19 −13internal/console/billingpass.go+2 −2internal/console/billingquantityaudit.go+11 −4internal/console/billingservice.go+20 −0internal/console/billingservice_test.go+9 −0internal/console/billingstore.go+27 −8
CI passed — run details
diff --git a/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md b/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md| index 578484619..dc3a71370 100644 |
| --- a/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md |
| +++ b/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md |
| @@ -146,7 +146,7 @@ func PublicPriceBook(book string) ([]byte, error) // the export below, byte-stab |
| New columns, both `TEXT NOT NULL DEFAULT ''`, on `billing_checkout_intents` and `billing_accounts`: `currency`, `price_book`. Empty in both means a legacy row. Assignments of a purchase priced from a book go in a new table, `billing_book_assignments`, with the same columns plus `currency` and `price_book` required non-empty; `billing_assignments` keeps only legacy assignments. The split is for rollback: a build from before books reads `billing_assignments` alone, so it finds no history for a book-priced purchase. For an account with both kinds of history it would find the legacy half, so a database trigger (`billing_months_no_legacy_calc_over_paid_book_history`; the earlier, wider `..._over_book_history` is dropped at migration) refuses to freeze a calculation whose top-level `v` is not 2 (read with `json_extract`) for any month into which an active book-priced assignment of the account was priced: its span, from its effective instant to the earliest of the next assignment's effective instant (by time then revision, over both tables), its paid-through instant and the month's end, has length inside the month, which is `AllowanceForMonth`'s own rule. A book assignment that never counted for a month (cancelled before its first reconciliation, or superseded in the same second) is left out, so this build's own version 1 for a legacy-only month after it still goes in. The environment and the binary change together, and no binary from before books runs against a database with a book-priced row (roll note in the dispositions). |
| -- **Checkout** (`POST /a/{sqid}/billing/checkout`): one form, with the country and currency controls, the plan, the postal code and the consent. The form carries what the page priced (`quoted_country`, `quoted_currency`) and whether the currency shown was a choice (`currency_chosen`). A post whose country or currency is not what the page priced, judged on what would be bought (the chosen currency, else the posted country's own), or whose quote the page did not sign (`quote_sig`: an HMAC over the session token, the account, the book, the quoted country, the quoted currency, the choice flag and the member count the page priced at, keyed by a secret derived from the billing secret key, which only the server holds), is not a purchase: it is answered with a redirect to the page quoting the posted country (and the posted currency when it is a choice), so nothing is bought at a price a page did not show this session. The member count the page priced at is compared again under the membership lock when the door charges; a count that moved is a conflict (`?error=conflict`, the existing banner), not a higher charge. A currency shown as the country's default and not chosen is given to the door as no currency, and the billing country's own currency is bought in; a chosen one (the URL's, the pricing page's, the account's, or changed on the form) is the purchase's. A currency field that is present and is not exactly `usd`, `eur` or `gbp` (no trimming, no case folding, not empty): 400, nothing is created, before the quote is looked at; on the page, a `?currency=` that names such a thing is said (`billing.currency.unknown`, unreviewed system copy like `billing.quote.unknown`) and the default shown is not a choice. The quote comparison is made on what would be bought (the chosen currency, else the posted country's own), so a form whose fields disagree with each other, a choice flag erased, or a form with no currency control against a page that priced a choice, is answered with the quote for what it would buy. The intent stores the chosen currency and book. A retry with a different currency while an intent is open is a different intent, exactly as a different plan is today. `POST /a/{sqid}/billing/quote` is the form's "Show prices for this country" button: the same form, answered with the same redirect, buying nothing; it asks for the session and the form's token, no step-up. |
| +- **Checkout** (`POST /a/{sqid}/billing/checkout`): one form, with the country and currency controls, the plan, the postal code and the consent. The form carries what the page priced (`quoted_country`, `quoted_currency`) and whether the currency shown was a choice (`currency_chosen`). A post whose country or currency is not what the page priced, judged on what would be bought (the chosen currency, else the posted country's own), or whose quote the page did not sign (`quote_sig`: an HMAC over the session token, the account, the book, the quoted country and its band at render time, the quoted currency, the choice flag and the member count the page priced at, keyed by a secret derived from the billing secret key, which only the server holds), is not a purchase: it is answered with a redirect to the page quoting the posted country (and the posted currency when it is a choice), so nothing is bought at a price a page did not show this session. The member count the page priced at is compared again under the membership lock when the door charges; a count that moved is a conflict (`?error=conflict`, the existing banner), not a higher charge. A currency shown as the country's default and not chosen is given to the door as no currency, and the billing country's own currency is bought in; a chosen one (the URL's, the pricing page's, the account's, or changed on the form) is the purchase's. A currency field that is present and is not exactly `usd`, `eur` or `gbp` (no trimming, no case folding, not empty): 400, nothing is created, before the quote is looked at; on the page, a `?currency=` that names such a thing is said (`billing.currency.unknown`, unreviewed system copy like `billing.quote.unknown`) and the default shown is not a choice. The quote comparison is made on what would be bought (the chosen currency, else the posted country's own), so a form whose fields disagree with each other, a choice flag erased, or a form with no currency control against a page that priced a choice, is answered with the quote for what it would buy. The intent stores the chosen currency and book. A retry with a different currency while an intent is open is a different intent, exactly as a different plan is today. `POST /a/{sqid}/billing/quote` is the form's "Show prices for this country" button: the same form, answered with the same redirect, buying nothing; it asks for the session and the form's token, no step-up. |
| - **A currency already held**: checkout is refused while the account's subscription has not ended (`canceled` or `incomplete_expired`), and after it has ended a checkout in any currency but the one bought in is refused, the billing country's default included; the page offers that currency alone, as a choice, for a purchase from before price books too (EUR). A change of currency waits on what Stripe permits for a customer whose subscription ended (test-mode item 4) and is then an operator's door, not the form's. |
| - **Stripe price identity**: the configured price for (book, plan, band, currency) must have that `currency`, `unit_amount` equal to the book's unit price, no `currency_options` but, at most, an entry under its own currency at its own amount and tax behaviour (which an expanded read of a single-currency price may carry), and metadata `price_book`, `currency`, `terms_version`, `plan`, `band` that all match. The checks that exist today (per-unit, yearly, licensed, exclusive tax, active, mode) stay. |
| - **Checkout Session**: created with `currency` and `adaptive_pricing[enabled]=false`, and with `currency`, `price_book` in the session and subscription metadata. A purchase with no book (only a console with no book configured opens one, and an intent from before books is replayed as that build sent it) is asked for exactly as before: no currency, Adaptive Pricing on. Every session the console creates, replays, lists, reads back or binds passes one check against its intent; one that is this intent's and not its purchase (another currency, a conversion, Adaptive Pricing) is expired and the intent retired, and one already paid is left for a person with a review filed once. A subscription whose price is not the configured one for its stored (book, plan, band, currency), or that the provider reports in another currency, is an identity mismatch. |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review7.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review7.md| new file mode 100644 |
| index 000000000..c9b55f368 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review7.md |
| @@ -0,0 +1,130 @@ |
| +**Sixth-review findings** |
| + |
| +All platform references below are at **`124eab27`**; site references are at **`9427c22`**. Console filenames are under `internal/console/`. The requested `124eab27..124eab27` delta is empty; I also inspected `b51c2a26..124eab27` against the embedded sixth-review reports. Nothing was modified; no tests or network commands were run. Test counterfactuals are deductions from source. |
| + |
| +| Sixth-review finding | Closed? | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **Opus P2-1 — Buyer-keyed quote signature** | **Yes.** `billingpage.go:440` derives the signing key from the billing secret and authenticates session, account, book, country, currency, choice flag and member count. | **No.** `billingcurrency_test.go:2014` attempts the current message signed with the disclosed token. Restoring that key makes the negative case fail. | |
| +| **Opus P3-1 — Account-wide deletion, unfinished quantity events, missing audit** | **Partly.** Retirement names the exact intent and records it; quantity events receive an outcome. That outcome remains invalid to two consumers, and an unbound refusal loses its session identity. Findings 1 and 3. | Removing event processing or the retirement record fails `billingcurrency_test.go:2822`. **Yes** for replacing the named deletion with account-only deletion: the repeated-resolution test returns before reaching deletion. The two remaining defects also pass. | |
| +| **Opus P3-2 — Incorrect authority-path comments** | **Partly.** The binding comment and CREATE-before-DROP explanation are corrected. `billingstore.go:202` still says “dropped first,” immediately contradicting the corrected paragraph. | **Yes.** These comments have no behavioral assertion. | |
| +| **Astra 1 — Resolution race deletes a newer intent** | **The reported race is closed.** Filing checks that the exact intent still exists inside its transaction (`billingstore.go:950`); resolution selects and deletes that identity (`:913`, `:929`). Retirement remains incomplete as above. | **No** for removing the existence check: the late filing at `billingcurrency_test.go:2852` reopens the review. **Yes** for removing only the deletion’s intent predicate. | |
| +| **Astra 2 — Forged quote signature** | **Yes**, as Opus P2-1. | **No**, as above. | |
| +| **Astra 3 — Membership growth changes the quoted total** | **Yes.** The authenticated member count is compared at `billingservice.go:413`; the resulting snapshot is checked again under the membership write lock by `billingstore.go:345`, `:372` and `billingmembership.go:533`. | **No.** The eleven-to-twelve-member case at `billingcurrency_test.go:2035` distinguishes removing the quote-count comparison. | |
| +| **Astra 4 — Pass uses a response-selected intent** | **Original single-response case closed; repeated-read case remains.** The pass now checks its original intent, but then calls a binder that rereads and independently selects an account. Finding 2. | **No** for removing the new pass guard: `billingcurrency_test.go:2688` catches that. **Yes** for the remaining second-read substitution. | |
| +| **Astra 5 — Failed pagination reports success** | **Yes.** The pager propagates command/JSON failures, validates the list envelope, and its callers stop on failure (`implementation/carloku-paid-launch/stripe-provision.sh:396`). | **No** for literally removing the asserted failure handling. **Yes** for behavioral breakage retaining the checked strings: `internal/plans/provision_test.go:27` does not execute the pager. | |
| + |
| +On the severity disagreements, **Astra’s P2 reading of the resolution race was right**: the account-only predicate was protected only until a stale binder reopened the resolved review. Opus’s sequential argument did not cover that interleaving. **Astra was also right about membership changing the displayed total** and about the pass selecting another account’s intent; these were behavioral gaps, not merely differences in wording. The ownership cases require inconsistent provider responses, rather than a demonstrated buyer-controlled exploit. Both reviewers correctly identified the signature forgery as P2. |
| + |
| +The sixth-review claims are assessed individually below. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P2 — Resolving a refused checkout leaves its quantity history permanently obstructing billing and account deletion.** |
| + |
| + **Where:** `billingquantityaudit.go:43`, `:51`; writer at `billingstore.go:922`. |
| + |
| + The processing trigger now accepts `checkout_refused_resolved`, but both the startup history audit and `quantityObligationTx` still recognize only: |
| + |
| + ```text |
| + charged, included, landed_unverified, checkout_expired, checkout_abandoned |
| + ``` |
| + |
| + **Scenario:** A live Community checkout has a membership event while pending. Its wrong-currency completion is reviewed and settled. Resolution marks that event `checkout_refused_resolved` and deletes the intent. |
| + |
| + Immediately afterward, `quantityObligationTx` treats the processed event as invalid. It blocks subsequent month freezes (`billingstore.go:1078`) and account retirement (`:1165`). On restart, the migration additionally writes `history_gap=1,state='history_unproven'`; ordinary summary refresh preserves that flag (`billingmembership.go:462`). |
| + |
| + The new test checks only that no events remain unprocessed (`billingcurrency_test.go:2833`). It never checks whether their outcome is accepted downstream. |
| + |
| + **Smallest acceptable fix:** Add the outcome to both remaining validators, preferably from one shared definition. Extend the resolution test through the obligation check and a second migration/startup, verifying that settled events neither hold the account nor create a history gap. |
| + |
| +2. **P2 — The pass’s ownership check does not protect the binder’s subsequent read.** |
| + |
| + **Where:** `billingpass.go:200`, `:259`; `billingservice.go:911`, `:918`, `:933`. |
| + |
| + **Scenario:** |
| + |
| + - Account A’s intent IA is bound to `cs_A`; account B has a ready, unbound intent IB. |
| + - The pass’s first read of `cs_A` returns A’s genuine completed session. Its new `ownSession(session, IA, A)` check passes. |
| + - `bindBillingCheckout` reads `cs_A` again. This response retains ID `cs_A`, but now carries B’s account reference, customer, intent IB and subscription. |
| + - The binder selects B from that response and checks ownership against IB. Because IB is unbound, the bound-session clause imposes no ID restriction. |
| + - It can bind `cs_A` to IB, or file B’s refusal review if the returned currency is wrong. |
| + |
| + The lost-response recovery branch has the same second-read dispatch at `billingpass.go:259`. There is no unique session-ID index preventing the second binding. |
| + |
| + The new two-account test supplies B’s identity on the **first** read, so it does not cover this sequence. |
| + |
| + **Smallest acceptable fix:** Carry the expected account and intent into binding from intent-driven callers. Validate every reread against that expected identity before selecting another account, reviewing or binding. Add a two-response case: correct A response first, B response second. |
| + |
| +3. **P3 — Retirement still loses the refused session identity when the intent was never bound.** |
| + |
| + **Where:** `billingstore.go:943`, `:913`, `:925`; caller at `billingservice.go:294`. |
| + |
| + `fileCheckoutRefusedReview` receives `sessionID` but never uses or stores it. Resolution instead copies `billing_checkout_intents.session_id`. |
| + |
| + **Scenario:** CreateCheckout’s response is lost. Recovery finds the intent’s completed session in the wrong currency. `refuseSession` files the intent-keyed review without binding the session. The intent therefore still has `session_id=''`. After settlement, resolution records an empty session ID in `billing_retired_intents`, then deletes the intent. The durable review and resolution contain the intent ID but no provider session or subscription ID. |
| + |
| + The audit assertion at `billingcurrency_test.go:2829` covers only an already-bound session. |
| + |
| + **Smallest acceptable fix:** Persist the observed refused session ID with the intent-keyed review, without treating it as an accepted binding, and copy that identity into the retirement record. Add an unbound, completed-refusal case. |
| + |
| +**Remaining requested paths** |
| + |
| +- **A — GBP purchase end to end.** `GET ?country=IN¤cy=gbp` renders band D, chosen GBP and a signed quote (`billingpage.go:265`). Personal is £7.70. The handler compares the effective currency and signed fields, then passes GBP and the quoted member count to checkout (`billinghandlers.go:81`, `:94`, `:107`). Checkout prices and stores GBP independently of the country’s band (`billingservice.go:417`, `:428`, `:522`). `checkoutParams:221` supplies fixed GBP; `internal/billingstripe/client.go:622` disables Adaptive Pricing. Binding compares price terms and fixed currency (`billingservice.go:945`); reconciliation checks subscription/item currency, intent terms and completed-session currency (`:748`, `:768`, `:789`). Account and assignment writes retain the book/currency and reject changes on the same subscription (`billingstore.go:594`, `:720`). History reads both fields (`billinginvoicestore.go:530`); `AllowanceForMonth` preserves one commercial identity (`internal/plans/month.go:174`). Calculation version 2 uses its GBP rates and floor (`billingcalculation.go:174`). Invoice parameters, recovery checks, line verification and hashes use the frozen currency (`billinginvoice.go:859`, `:886`, `:901`, `:961`, `:1024`). Quantity reads and update acknowledgements check the stored price and currency (`billingquantity.go:1014`, `:1105`). **I found no ordinary path converting that purchase or repricing it in another currency.** |
| + |
| +- **B/M/Q/U — Form and signature.** Changed country/currency, fabricated agreeing quote fields, and changed choice flags cannot buy without a valid server-issued signature. A signature is reusable for the same authenticated tuple; it is not invalidated merely because another tab renders another quote. That is safe: the old tab still buys its own displayed quote, subject to current membership and held-currency checks. Another account, session, book, country, currency, flag or member count changes the MAC. A changed session token fails CSRF or signature verification. The disclosed fields do not reveal the derived key (`billingpage.go:440`). Plan is omitted because all offered plans are rendered; quantity follows from signed membership and plan minimum. Postal code affects tax, while the page explicitly quotes before VAT (`templates/billing.html:42`). |
| + |
| + A present currency field must contain exactly one supported lowercase value; empty, repeated, padded and uppercase values receive 400 on both doors after their preceding gates (`billinghandlers.go:166`). A missing control means no explicit choice: the effective country default must still match the signed quote. A legacy console uses EUR without a control. Forging the flag cannot authenticate a different quoted state. |
| + |
| +- **B/C/O — Defaults, cookies and held currency.** An absent URL parameter falls back to its cookie; present-empty suppresses fallback and clears it; invalid input sets nothing (`billingcurrency.go:43`, `billingpage.go:232`, `:303`). Consequently, an old valid cookie can reappear on a later parameter-absent request after an **invalid** parameter, but not after a successfully processed explicit clear. Accepted POSTs do not consult these cookies. A held currency overrides URL/cookie suggestions, including legacy EUR (`billingpage.go:297`), and checkout refuses a different purchase currency (`billingservice.go:509`). The quote door checks session, token and membership, then constructs only the escaped account billing URL (`billinghandlers.go:127`); it performs no billing mutation. |
| + |
| +- **D/L/R/S/V/W — Ownership and refusal resolution.** The ownership predicate checks nonempty ID, runtime mode, intent metadata, account reference, known customer and bound ID (`billingservice.go:259`). Both create sites, interactive resume reads and listing recovery apply it before consequential action. Finding 2 prevents a universal closure claim. The already-recorded-subscription shortcut at `billingservice.go:930` also precedes `ownSession`; it returns an existing identity for independently checked reconciliation rather than recording a new purchase. |
| + |
| + Owned completed refusals are not expired. Their review is deduplicated by intent, and filing after that intent disappears does nothing. Exact-intent retirement and quantity-event updates share the resolution transaction. Repeating a resolved occurrence returns before touching a newer intent. The processing trigger is replaced when its stored text differs (`billingquantityaudit.go:37`), including on an older database; its downstream consumers are finding 1. |
| + |
| +- **E/H/N/T — Barrier and rollout.** For valid writer-produced history, the trigger matches the positive paid-span rule: |
| + |
| + ```text |
| + max(effective_at, month_start) |
| + < min(next_assignment_effective_at, period_end, month_end) |
| + ``` |
| + |
| + `billingstore.go:208` and `internal/plans/month.go:146` agree on both-table ordering, higher revisions in the same second, free/inactive assignments terminating earlier spans, and paid-through clipping. I found no requested counterexample allowing an older version-1 freeze over a span this build prices from the book, or refusing this build’s legitimate legacy-only month. Renewal and quantity dating change assignment effectiveness; the assignment writer copies the provider’s period end, which history exposes as `PaidThrough` (`billingstore.go:608`, `:645`, `:720`; `billinginvoicestore.go:542`). |
| + |
| + Startup creates the new trigger before transactionally checking its presence and dropping the old one (`billingstore.go:208`, `:788`). Missing-new failure preserves the old trigger; a second start is idempotent. An interruption can leave both, but this replacement sequence does not remove the last existing barrier. The new binary migrates an old database before using the new columns. Rollback still requires coordinated environment changes and the documented prohibition on pre-book binaries after book purchases. |
| + |
| +- **F/G/J — Figures, validation and export.** Current resource figures divide exactly at every valid quantity because each per-member figure remains integral after band scaling. Unavailable Personal is displayed at quantity one; teams use their charged quantity (`billingpage.go:402`, `:423`). Band A is asserted verbatim at `billingcurrency_test.go:2560`; the formatter’s one-third example demonstrates truncation, but current terms do not produce that in a displayed share. Book validation checks complete price matrices, checked rounding arithmetic, currency coverage, duplicate keys and legacy EUR equality (`internal/plans/pricebook.go:149`, `:231`). Configuration requires the current catalog and permits complete additional catalogs with unique price IDs (`cmd/console/billing.go:106`, `:134`). Provisioning checks at least 36 configured keys and permits the same own-only currency-option shape (`stripe-provision.sh:424`, `:434`). The public export exposes commercial data, not account/provider secrets. |
| + |
| +- **K — Pricing site.** The pinned site book is byte-identical to the platform export. Exact rate formatting, band-scaled inclusions and divisibility/precision checks agree with current terms (`copy-review/pricing/build.mjs:48`, `:103`, `:160`). Approved-string figures are checked against the export; country options come from its country list and carry its defaults (`:134`, `:144`, `:208`). Signup links always carry both parameters; currency is populated only for an explicit choice (`src/site.js:75`). The first-line draft marker and exact `"1"` override are present, and ordinary `npm run build` runs the checker first. A locale-only GBP display can lead to console USD with empty suggestions; that is the behavior ruling 9 requires, not a transmitted GBP choice. |
| + |
| +**Claims the code contradicts** |
| + |
| +| Sixth-review claim or carried claim | Assessment at `124eab27` | |
| +|---|---| |
| +| **Quote provenance is false because the buyer holds the key.** | **Closed.** The server-derived key fixes this. The old token-keyed and current-message token-keyed forgeries are both tested. | |
| +| **Quantity is an unauthenticated price input.** | **Closed.** Membership is signed and checked through the locked intent snapshot. | |
| +| **Every session passes the same ownership check before consequential action.** | **Still overstated.** Finding 2 changes the expected identity between reads. The already-recorded shortcut is also an explicit exception. | |
| +| **A late refusal filing can reopen a resolved review.** | **Closed for the reported race.** The exact-intent existence check is transactional. | |
| +| **Resolution records the refused session and fully settles its quantity events.** | **Not fully true.** Findings 1 and 3 distinguish processing an event from accepting its outcome, and bound from unbound session audit. | |
| +| **Trigger CREATE and DROP share one transaction.** | The corrected paragraph is accurate, but `billingstore.go:202` and dispositions line 116 still say the old trigger is dropped first. | |
| +| **Every fix has a test that fails without it.** | **Still false as a blanket statement.** The migration test at `billingcurrency_test.go:2425` checks uninterrupted final state, so unsafe DROP-before-CREATE also passes. The named-delete mutation survives. Negative webhook cases at `:2639` do not assert response status or execution and can pass a no-op webhook; the separate fresh-review test does catch a globally disabled webhook. | |
| +| **Provisioning tests read generated checks.** | **False in dispositions lines 79 and 120.** They read shell source. Review-five line 145 now describes this correctly. | |
| +| **Removing the inclusion assertion is killed.** | **False in dispositions line 143.** Removing assertions over the current exact data still passes. The data mutation described at line 135 is a different, meaningful check. | |
| +| **Review four unanimously closed the third review.** | **False in dispositions line 108.** The embedded Astra review-four table explicitly records partial closure for ownership, crafted forms, trigger migration and other cases. | |
| +| **Invalid currency receives 400 “before anything.”** | **Overstated in dispositions line 114.** Checkout authentication/step-up and consent handling precede currency validation. Nothing is purchased on those earlier exits. | |
| + |
| +The review-three default-as-explicit mutation is correctly described as equivalent for accepted ordinary forms. The listed behavioral mutations now generally have distinguishing assertions, but source inspection cannot establish that historical mutation runs occurred. The review-four table’s eight mutations likewise do not establish its broader unanimity claim. |
| + |
| +I found no concrete calendar-date, test-order, temporary-root or shared-cookie-jar failure in the inspected additions. Authentication/webhook timestamps use the current clock; the one-form helper deliberately has no cookie jar. The site’s “platform export” test checks schema and a provenance-text pattern rather than cross-repository equality (`test/pricing-pricebook.test.mjs:32`); the pinned copies nevertheless match. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +1. **Catalog identity:** Read all 36 prices with expanded currency options. Confirm exact amounts, currency, metadata, annual licensed recurrence and exclusive tax. Exercise catalogs exceeding 100 objects and failed later pages; failures must stop verification. |
| +2. **Checkout matrix:** Complete USD/EUR/GBP purchases across IE, GB, US, IN and JP, including IN/GBP. Confirm unit × quantity, session/subscription/item currency, disabled Adaptive Pricing and no conversion. Record Stripe’s response to incompatible price/session currency. |
| +3. **Renewals and quantity changes:** Exercise both proration policies and renewal in each currency. Confirm currency, amounts, effective dates and unchanged price identity. Run the quantity-fence sandbox verification before enabling its switch. |
| +4. **Overages:** Exercise all four meters, rounding boundaries and invoice floors through draft, item creation, finalization and payment. Confirm each line and subtotal against the frozen calculation, including canceled subscriptions and automatic tax. |
| +5. **Recovery:** Lose responses around checkout and invoice operations, then replay and recover by listing within and beyond retention windows. Confirm original identities and no duplicate charge, including legacy request compatibility. |
| +6. **Customer currency constraints:** Establish behavior during a live subscription, after cancellation and with outstanding older invoices. Confirm the console’s held-currency rule remains effective. |
| +7. **Refusals and events:** Record open/completed-session expiry responses; replay and reorder webhooks before and after refusal resolution. Confirm one review, exact-intent retirement and no renewed accounting effect. |
| +8. **Discounts:** Exercise incompatible fixed-amount currency discounts, the percentage promotion and house coupon. Confirm actual charged currency and resulting amounts. |
| + |
| +**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 2e327eedb..06ff95fce 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -105,13 +105,13 @@ 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 every third-review finding closed in the code and no path that stores or sends money in a currency other than the purchase's; the disputed trigger reading went Astra's way. Every finding of either that is marked fixed below has a test that fails without it; eight mutants, one per fix, were killed in a scratch copy; 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 finding of either that is marked fixed below has a test that fails without it; eight mutants, one per fix, were killed in a scratch copy; two items are left open and say so. |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| | Astra 1 (P2): `ownSession` did not compare the customer, so a session carrying the intent's id but another customer could be expired, or saved, before a later check refused it. | Fixed: `ownSession` also requires the intent's customer (once it has one) and, once bound, the bound session's id; `intentSession` starts from it; `bindBillingCheckout` checks the customer before anything. | `billingservice.go` | `TestCurrencyRefusalExpiresOnlyTheIntentsOwnSession` (another customer, in another currency and in the right one: zero expiries, nothing kept) | |
| | Astra 2 (P2): posted quote fields and the choice flag could make the door buy a currency the page did not show (the flag erased on a chosen GBP page bought India's dollar). | Fixed: the handler compares the quote with what would be bought (the chosen currency, else the posted country's own), not with the posted field. A form whose fields disagree with each other, or has no currency control against a chosen quote, is answered with the quote for what it would buy. Fields forged to agree with each other bought exactly what they described, which the buyer saw at the provider; review five held that the console's own quote rule asks for more, and the quote is signed from review five on (below). | `billinghandlers.go` | `TestBillingFormPostsWhatItShows` ("a choice flag erased buys nothing the page did not show") | |
| -| Astra 3 (P2), Opus P3-5: an invalid currency reached the quote redirect instead of a 400, `" "` was trimmed into absence, and an unsold `?currency=` was silently replaced on the page. | Fixed: a present currency field must be exactly a currency sold (no trimming, no folding, not empty), else 400 before anything, on the checkout and the quote door; the page says when `?currency=` names something not sold and does not post the default as a choice. | `billinghandlers.go` `billingFormChoice`; `billingpage.go`; `billing.html` (`billing.currency.unknown`, unreviewed system copy, flagged) | `TestBillingFormPostsWhatItShows` ("a currency that is not sold is refused before anything") | |
| +| Astra 3 (P2), Opus P3-5: an invalid currency reached the quote redirect instead of a 400, `" "` was trimmed into absence, and an unsold `?currency=` was silently replaced on the page. | Fixed: a present currency field must be exactly a currency sold (no trimming, no folding, not empty), else 400 before the quote is looked at or anything is created (the session, the form's token, the step-up and the consent are checked before it, and buy nothing), on the checkout and the quote door; the page says when `?currency=` names something not sold and does not post the default as a choice. | `billinghandlers.go` `billingFormChoice`; `billingpage.go`; `billing.html` (`billing.currency.unknown`, unreviewed system copy, flagged) | `TestBillingFormPostsWhatItShows` ("a currency that is not sold is refused before anything") | |
| | Astra 4 (P2): the trigger counted an active book row superseded in the same second, which `AllowanceForMonth` skips. | Fixed: the trigger's span ends at the earliest of the next assignment's effective instant (time, then revision, over both tables), `period_end` and the month's end, and must have length inside the month: `AllowanceForMonth`'s rule. | `billingstore.go` | `TestBarrierTriggerLetsThisBuildsLegacyMonthsThrough` ("superseded in the same second") | |
| | Astra 5 (P2), Opus P3-1: a database opened by an earlier build kept the old trigger. | Fixed: the trigger has a new name and the old one is dropped first at migration. | `billingstore.go` | same test ("a database that carries the earlier, wider trigger": the old body installed, migration run, the new name alone remains, the legacy month freezes) | |
| | Astra 6 (P2): a bound completion whose metadata changed with its currency returned before the review. | Fixed: an owned completed session on other price terms or in another currency files the review; the webhook path is now exercised and `occurrences` stays 1. | `billingservice.go` `bindBillingCheckout` | `TestBoundSessionCompletedInAnotherCurrencyIsReviewedOnce` (two passes, a binding, two webhooks: one review, one occurrence; dollars throughout, metadata included) | |
| @@ -142,7 +142,7 @@ Reports: `OPUS-REVIEW-multi-currency-review5.md`, `CODEX-FINDINGS-multi-currency |
| | Astra 4 (P3), Opus carried P3-3: resolving a refused, paid session's review left the intent standing, so the next pass reopened the review. | Fixed: resolving a review of that reason retires the account's pending intent in the same transaction; the next pass and webhook find nothing to bind and the review stays resolved. | `billingstore.go` `resolveBillingReview` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` | |
| | Opus P3-3: one clause of the inclusions test was a tautology. | Fixed: the test asserts, for every paid plan, band factor and axis, that the figure times the factor is whole in base units and that the per-member share shows in three decimals of its unit; the tautology is gone; the page comment says what the test checks. | `billingcurrency_test.go`, `billingpage.go` | the test itself (a mutant removing the check is killed) | |
| | Opus P3-4, Astra claims: the webhook leg ran after the review was already open; the bind customer check and three `ownSession` clauses were untested. | Fixed: the webhook alone files the review on a fresh fixture; each `ownSession` clause has a case; the bound-id and customer paths are exercised. | tests | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution`, `TestOwnSessionClauses`, `TestOwnershipIsEnforcedOnEveryPath` | |
| -| Opus P3-5, Astra 6 (P3): Check 2 counted products account-wide and read one page of 100 prices. | Fixed: every page is read (`starting_after`) and products and prices are counted by their metadata (`terms_version`, `price_book`); the plans test reads the script's text for the pager and the filters (it does not run them: no Stripe here). | `stripe-provision.sh`; `plans/provision_test.go` | `TestStripeProvisionScriptMatchesThePriceBook` | |
| +| Opus P3-5, Astra 6 (P3): Check 2 counted products account-wide and read one page of 100 prices. | Fixed: every page is read (`starting_after`) and products and prices are counted by their metadata (`terms_version`, `price_book`); the plans test reads the script's text for the pager and the filters (it does not run them: no Stripe here; the rehearsal does). | `stripe-provision.sh`; `plans/provision_test.go` | `TestStripeProvisionScriptMatchesThePriceBook` | |
| | Astra 7 (P3): the verifier refused an own-only option whose `tax_behavior` is the empty string, which the console accepts. | Fixed: the verifier accepts null, empty or `exclusive`, as the console does. | `stripe-provision.sh` | the same test (the accepted shapes are in the asserted expression) | |
| | Astra 5 (P3), Opus P3-6: `npm run build` bypassed the draft check. | Fixed on the site (`9427c22`): the ordinary build runs the check first; the test reads both scripts. | site `package.json` | site test | |
| | Opus claims: the design's "once"; the dispositions' "every finding fixed", the signing argument, the coverage claim; the design's Adaptive Pricing and no-options absolutes. | Corrected in this commit. | design, this file | | |
| @@ -158,7 +158,7 @@ Reports: `OPUS-REVIEW-multi-currency-review6.md`, `CODEX-FINDINGS-multi-currency |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| | Opus P2-1, Astra 2 (P2): the quote signature was keyed by the buyer's own session token, so the buyer could sign any quote from what the form disclosed. | Fixed: the key is a secret derived from the billing secret key (as `locationDigest` derives its own), which only the server holds; the MAC covers the session token, the account, the book, the quoted country, the quoted currency, the choice flag and the member count. | `billingpage.go` `billingQuoteSignature`, `billinghandlers.go` | `TestBillingFormPostsWhatItShows` (a forgery computed exactly as the fifth build signed, from the form's own csrf: refused; the rendered quote buys) | |
| -| Astra 3 (P2): a member who joined between the page and the post bought a higher team total under a valid quote. | Fixed: the page's member count is in the signature and the form; the door compares it with the count it charges, under the membership lock, and a count that moved is `errBillingConflict`, which the page already explains ("the account changed while you were on this page, for example a member was added"); a forged count fails the signature. | `billingservice.go` `startBillingCheckoutQuoted`, `billinghandlers.go` | `TestBillingFormPostsWhatItShows` ("a member who joins between the page and the post": eleven priced, a twelfth joins, the post is a conflict; the refreshed page buys twelve) | |
| +| Astra 3 (P2): a member who joined between the page and the post bought a higher team total under a valid quote. | Fixed: the page's member count is in the signature and the form; the door compares it with the count it charges (under the billing lock; the membership-locked check is the intent snapshot's, against the door's own count), and a count that moved is `errBillingConflict`, which the page already explains ("the account changed while you were on this page, for example a member was added"); a forged count fails the signature. | `billingservice.go` `startBillingCheckoutQuoted`, `billinghandlers.go` | `TestBillingFormPostsWhatItShows` ("a member who joins between the page and the post": eleven priced, a twelfth joins, the post is a conflict; the refreshed page buys twelve) | |
| | Astra 1 (P2), Opus P3-1: the refusal review was keyed by the subscription, so a binding paused across a resolution could reopen it, and the resolution deleted the account's intent by account alone, closed none of its quantity events and recorded nothing of it. | Fixed: the review is keyed by the intent (its subscription field holds the intent id) and is filed only while that intent stands, in one transaction; the resolution retires exactly that intent, closes its quantity events (`checkout_refused_resolved`) and records what it was in `billing_retired_intents`; a later intent is untouched. | `billingstore.go` `fileCheckoutRefusedReview`, `resolveBillingReview` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` (the record, the events, a new purchase after the resolution, a late filing for the retired intent files nothing, a second resolution named for the old intent leaves the new one) | |
| | Astra 4 (P2): the pass judged ownership against the intent the response named, not the one it was working on. | Fixed: the pass applies `ownSession` to the session it read against the intent it is working on, with that intent's account, before dispatching on status. | `billingpass.go` | `TestOwnershipIsEnforcedOnEveryPath` ("the bound session answered with another intent's everything") | |
| | Astra 5 (P3): a failed page in the provisioning pager could still print "counts ok". | Fixed: a page that fails, or is not a list page, fails the whole; the plans test reads the script's text for it. | `stripe-provision.sh` | `TestStripeProvisionScriptMatchesThePriceBook` | |
| @@ -170,3 +170,21 @@ Mutation checks on these fixes in a scratch copy: the quote keyed by the session |
| Open after this round: the site's `bands.p3` sentence; `billing.quote.unknown` and `billing.currency.unknown` as unreviewed system copy; everything that needs Stripe test mode. |
| The fixes are in the commit that adds this section and have not been read by a reviewer. |
| + |
| +## Review seven (at 124eab27; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review7.md`, `CODEX-FINDINGS-multi-currency-review7.md`. The two agree on every finding this round, including the one P2: the outcome the resolution writes was admitted by the processing guard and unknown to the two other validators, so a resolved refusal held the account for good with nothing open to clear it. Both closed the sixth round's quote-key and quantity findings. Everything of either is fixed below with a test that fails without it; four mutants were killed in a scratch copy. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P2-1, Astra 1 (P2): `checkout_refused_resolved` was in the processing guard's list and not in the startup sweep's or `quantityObligationTx`'s, so a resolved refusal left the account held (no month frozen, no deletion) with no review open. | Fixed: one Go constant holds the admitted outcomes and all three statements use it. The test follows the resolution through the obligation check, a month's freeze and a second start. | `billingquantityaudit.go` `quantityOutcomesSQL` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` | |
| +| Opus P3-1, Astra 2 (P2): the pass's ownership test covered its own read, and the binder's second read derived the account from the response again. | Fixed: intent-driven callers bind through `bindBillingCheckoutFor`, which judges the session it reads against the intent it was given, by that intent's account, before any account is derived; the webhook, which knows no intent, is judged against the intent the response names, as before. | `billingservice.go`, `billingpass.go` | `TestBindersSecondReadIsJudgedAgainstTheIntent` (a first answer that is A's own, a second that is B's under the same id: nothing for either account) | |
| +| Opus P3-2, Astra 3 (P3): a refused session found by the listing, on an intent never bound, was recorded nowhere by its id. | Fixed: the review carries the session's id as its `reference` (a new column), and the retirement record takes it when the intent had no bound session. | `billingstore.go` | `TestUnboundRefusedSessionIsRecordedByItsId` | |
| +| Opus P3-3: the band is a price input the quote signature did not cover. | Fixed: the quoted country's band at render time is in the signature; the door recomputes it from the band map it has, so a map changed between render and post is a new quote. | `billingpage.go`, `billinghandlers.go` | `TestBillingFormPostsWhatItShows` ("a band changed by the operator") | |
| +| Opus claims, Astra claims: the trigger comment ("dropped first"); `fileCheckoutRefusedReview`'s unused parameter; the test comment on the second resolution; "generated checks" in two rows; the review-four unanimity sentence; "before anything"; the inclusion-mutation wording; "under the membership lock" (the door compares under the billing lock, the membership-locked check is the intent snapshot's). | Corrected in this commit, this file and the comments. | | | |
| + |
| +Mutation checks in a scratch copy: the new outcome left out of the obligation list; the binder ignoring the expected intent; the refused session's id not recorded; the band left out of the signature: all killed. |
| + |
| +Open after this round: the site's `bands.p3` sentence; `billing.quote.unknown` and `billing.currency.unknown` as unreviewed system copy; everything that needs Stripe test mode. Nothing else on this branch is marked open. |
| + |
| +The fixes are in the commit that adds this section and have not been read by a reviewer. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review7.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review7.md| new file mode 100644 |
| index 000000000..e8d858daf |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review7.md |
| @@ -0,0 +1,130 @@ |
| +# Seventh review of `feat/carloku-multi-currency-20261001` at `124eab27` (base `12e70d95`) |
| + |
| +**Verdict: accept with fixes.** One new P2: the refusal resolution closes quantity events with an outcome that two other outcome lists don't include. Any account it touches is then held for good, and there is no review left to clear the hold. |
| + |
| +I found no path that computes, sends, verifies or stores money in a currency other than the purchase's stored one. |
| + |
| +How I worked: |
| +- I read source only, with `git show 124eab27:`. The pricing site is unchanged at `9427c22`. |
| +- I modified nothing and ran no tests. |
| +- The requested delta `124eab27..124eab27` is empty. I read `b51c2a26..124eab27` as the sixth-round fixes. |
| +- `internal/plans` and the book are unchanged in this round. The trigger body and `retireEarlierBarrierTrigger` are unchanged apart from comments. |
| + |
| +## Sixth-review findings |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P2-1 / Astra 2**: the quote MAC was keyed by the buyer's own token | **Closed.** The key is derived from `cfg.SecretKey` under `carloku/billing/quote/v1` (`billingpage.go:440-449`). The MAC covers the session token, sqid, book, country, currency, flag and member count. `SecretKey` must be non-empty (`billingservice.go:93`). A closed console never signs: `CanChoose` is set only inside `!v.Closed`. | **No.** Both forgeries in `TestBillingFormPostsWhatItShows`, one in the old format and one in the new format keyed by the token, are accepted by a revert to the token key. | |
| +| **Astra 3**: membership changed between page and post | **Closed.** `quoted_members` is signed and compared at the door (`billingservice.go:413`). The door's count then has to match the count `checkIntentMembershipTx` takes under the membership lock (`billingmembership.go:542`), so the chain from page to commit holds. | **No.** Without the compare, the twelfth-member post opens a 12-seat intent. Without members in the MAC, the forged `quoted_members=12` buys. | |
| +| **Opus P3-1 / Astra 1**: the resolution deleted the account's intent unnamed, skipped its bookkeeping, and could reopen | **Closed as specified, with a new defect in the bookkeeping (P2-1 below).** The review is keyed by intent id. Filing checks that the intent still stands, in one transaction (`billingstore.go:943-967`). The delete is named, the events are closed and the intent is recorded in `billing_retired_intents`. | The late-filing assertion **fails** without the "stands" check. The retired-record and events-closed assertions **fail** without their code. "A second resolution leaves the new intent" **passes without the fix**: the repeat resolution returns early at `resolvedAt != 0`, before any DELETE (`:2855-2862`). The dispositions admit the account-wide delete mutant survives. | |
| +| **Astra 4**: the pass judged ownership against an intent the response named | **Closed for the pass's own read** (`billingpass.go:191-197`). A second read inside bind is still outside it (P3-1). | **No.** In the two-account case, B's unbound intent is bound to `cs_A` without the fix. | |
| +| **Astra 5**: a failed page could still print "counts ok" | **Closed** in the script: the `stripe get` status, the list envelope, the `jq` steps and the `exit 1` at the call site are all checked. | **Yes for behaviour.** The test checks two substrings only. | |
| +| **Opus P3-2**: stale comments | **Partly.** The bind comment is fixed. `billingstore.go:200-202` still says the old trigger "is dropped first", and the parenthesis right after it says the CREATE runs first. | n/a | |
| + |
| +**Where the reviewers disagreed in round six, the code says Astra was right on all three.** |
| +- **Resolution race.** At `b51c2a26`, `fileBillingReviewOnce` had no intent check, and resolution takes no billing lock (`billingoperator.go:221`). So reopen, then a second intent, then the account-wide DELETE was reachable. It would orphan a payable session: money taken and nothing granted. That is P2. My round-six P3 rested on invariants that the race breaks. |
| +- **Quantity.** Ruling 9's "nothing bought at a price the page did not show" applies; Stripe showing the amount later doesn't satisfy it. P2. |
| +- **Pass ownership.** Round six's "the bind's bound-id clause catches it" was wrong. Bind judged against B's unbound intent, which accepts any id. |
| + |
| +## Findings |
| + |
| +### P2-1. A resolved refusal leaves the account permanently held, with nothing open to explain it |
| +- **Where:** |
| + - `billingstore.go:922` writes `processing_outcome='checkout_refused_resolved'`. |
| + - The guard admits it (`billingquantityaudit.go:22`). The two sibling lists were not updated: |
| + - `quantityObligationTx` (`billingquantityaudit.go:51`) |
| + - the startup `history_unproven` sweep (`:43`) |
| +- **Scenario:** |
| + 1. A Community intent I1 is pending, and a member joins. That creates a quantity event on I1. |
| + 2. The session completes in EUR. The review is filed, then refunded and resolved, and the event is closed as `checkout_refused_resolved`. |
| + 3. From then on `quantityObligationTx` returns held. `freezeBillingMonth` refuses every new month with `errBillingReview` (`billingstore.go:1078`), so none of this account's later overage invoices are ever frozen or sent. `checkBillingRetirement` refuses deletion. On every start, `:43` marks the account's quantity changes `history_unproven`. |
| + 4. No review is open, so the operator has nothing to resolve. The events are append-only, so nothing clears the hold. |
| +- **Before this round the result was the same:** an unprocessed addition also held the account. The fix moved the hold; it didn't remove it. |
| +- **Smallest fix:** |
| + - Add `'checkout_refused_resolved'` to both lists. Better: one Go constant for the outcome list, used by all three statements. |
| + - Extend `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` to assert that `quantityObligationTx` is false after the resolution, and that a month freezes for the later purchase. |
| + |
| +### P3-1. Bind re-reads the session and still derives the account from the response |
| +- **Where:** `billingpass.go:200` passes only `session.ID` to `bindBillingCheckout`. Bind does its own `GetCheckout` (`billingservice.go:911`), takes the account from `metadata.account_sqid` (`:918`) and judges `ownSession` against that account's intent (`:942`). |
| +- **Scenario:** |
| + 1. The pass's read of `cs_A` is A's own and passes. |
| + 2. Bind's second read returns `cs_A` with B's sqid, intent and customer, and B's intent is unbound. |
| + 3. Bind binds B's intent to `cs_A`, then the pass reconciles it. |
| +- This needs two inconsistent provider answers in a row. |
| +- **Fix:** give bind the expected account id (the pass's intent's) and refuse when the account it derives differs. Alternatively, bind from the session the pass has already checked. |
| + |
| +### P3-2. The refused session's id is recorded nowhere when the intent was unbound |
| +- **Where:** |
| + - `fileCheckoutRefusedReview` ignores its `sessionID` parameter (`billingstore.go:943`). |
| + - The listing path refuses a paid session on an unbound intent without saving it (`billingservice.go:1190`). |
| + - The resolution copies `session_id` from the intent row (`:913`). |
| +- **Scenario:** |
| + 1. A create response is lost. |
| + 2. The pass's listing finds a paid EUR session and files the review. |
| + 3. The operator page shows only the intent id, in the "subscription" column. |
| + 4. On resolution, `billing_retired_intents.session_id=''` and the event reference is the literal `checkout_refused_resolved`. |
| + 5. The operator has to find the session in Stripe by its `checkout_intent` metadata. Round six's "what is lost: the session id" still holds on this path. |
| +- **Fix:** store the session id on the review, for example as a column or in the note, and write it into `billing_retired_intents`. Optionally also record it on the intent (`session_id` is still `''`) before filing. |
| + |
| +### P3-3. The band map is a price input the quote MAC does not cover |
| +- **Where:** `billingpage.go:267`. The band comes from `cfg.CountryBands` at both render and post. |
| +- **Scenario:** an operator changes a country's band and restarts. A form rendered before the restart still verifies and buys the new band's price. |
| +- **Fix:** add the band to the MAC message. |
| + |
| +Plan and postal are not price inputs in this sense: |
| +- **Plan** chooses among rows the page showed. An unavailable Personal row is refused by `checkIntentMembershipTx`'s `MaxQuantity` check. |
| +- **Postal** affects only tax. |
| + |
| +## Section notes |
| +- **A.** The GBP path from `?country=IN¤cy=gbp` to quantity worker is unchanged from round six. The only new step is the member-count compare. |
| +- **B / M.** The rules for a present, empty or repeated `currency` field are unchanged. The quote door is still an escaped redirect to the account's own billing page and writes nothing. A held currency, legacy EUR included, is offered alone, signed as chosen, and refused otherwise at `billingservice.go:525`. |
| +- **C.** The cookies are unchanged. Present-empty clears, absent falls back, invalid sets nothing. As Astra notes, an earlier valid cookie stays after an invalid parameter and shows again on a later GET with no parameter. Checkout never reads a cookie. |
| +- **D / L / R / W.** `ownSession` now gates every read, listing and webhook-to-bind path, apart from P3-1 and bind's early return for a subscription already recorded (benign, as before). |
| +- **E / N / T.** No change. Both the old and the new trigger can be present at once, and that fails closed. Neither being present is unreachable. |
| +- **F.** No change. |
| +- **G.** No change. |
| +- **H.** The new table is ignored by older binaries. An older binary restores its own guard text. Rows already closed with the new outcome are untouched, because the guard is `BEFORE UPDATE` only. |
| +- **J.** No change. |
| +- **K.** The site is unchanged at `9427c22`. |
| +- **O.** Legacy EUR is held and signed as EUR. |
| +- **P.** The review-four table is unchanged since round six. |
| +- **Q / U.** |
| + - A signature is bound to the session, account, book, quote and member count. |
| + - It can be replayed only for the identical tuple, which is the same price. |
| + - After a token change, the stale form fails csrf with a 403 first. |
| + - Rotating the Stripe key invalidates outstanding quotes, which then redirect harmlessly. |
| +- **S / V.** |
| + - The resolution retires only `(account_id, id = review key)`. |
| + - Filing for a retired intent files nothing. |
| + - SQLite serialises the filing transaction against the resolution: a stale snapshot fails to upgrade. |
| + - The guard is replaced in one `BEGIN IMMEDIATE` when its stored text differs. Its siblings were not updated (P2-1). |
| + |
| +## Claims the code contradicts |
| +- **Dispositions, review six:** |
| + - "the quantity events left open … killed": the events are now closed, but the account is held anyway (P2-1). |
| + - "Everything … is fixed below with a test that fails without it": the second-resolution assertion is vacuous, and the account-wide-delete mutant survives (admitted). |
| +- **Test comment** at `billingcurrency_test.go:2840-2842`, "a resolution named for the old intent touches the new one not at all": it returns before reaching the delete. |
| +- **Dispositions / design**, "compares it … under the membership lock": the door compares under the billing lock. The membership-locked check is `checkIntentMembershipTx`'s, against the door's own count. The effect is the same. |
| +- **Dispositions**, "records what it was": it records an empty session id on the unbound path (P3-2). |
| +- **`billingstore.go:200-202`**, "it is dropped first": the old trigger is dropped second. |
| +- **`fileCheckoutRefusedReview`'s doc comment** implies the session is recorded; its `sessionID` parameter is unused. |
| + |
| +## Before enabling: Stripe test mode |
| +Round six's eleven items stand. In short: |
| +1. Read all 36 prices with `expand[]=currency_options`. Run the pager against more than 100 prices, with a failed second page. |
| +2. A USD, EUR and GBP session for buyers in IE, GB, US, IN and JP, including IN with GBP. Expect a fixed currency, Adaptive Pricing off, no conversion, and unit × quantity. |
| +3. A session whose currency differs from its price's. Record the refusal shape. |
| +4. The completed subscription's currency and metadata. |
| +5. The customer currency lock: during a live subscription and after cancellation. |
| +6. GBP and USD overage invoices with only the IE tax registrations. |
| +7. GBP proration and renewal. |
| +8. Lost-response replays, and listing scoped by customer. |
| +9. `ExpireCheckout` on a completed session. |
| +10. A replayed `checkout.session.completed` after a refusal was resolved. Expect no review and nothing recorded. |
| +11. Promotion codes against fixed currencies. |
| + |
| +Two additions: |
| +12. A paid wrong-currency session reached through the listing, then resolved. Confirm the account's next month freezes and invoices (P2-1), and that the session id is findable (P3-2). |
| +13. A webhook and a pass meeting the same refused session concurrently. Expect a single review keyed by the intent. |
| + |
| +**Verdict: accept with fixes.** Fix P2-1 (two outcome lists and a test) before enabling. The P3s can follow. |
diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go| index 9cd07af7e..f90cb9891 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -203,7 +203,7 @@ func TestCheckoutFormCurrencyField(t *testing.T) { |
| // The quote as the page would have signed it for this session, this |
| // account and its one member. |
| form.Set("quoted_members", "1") |
| - form.Set("quote_sig", fx.srv.billingQuoteSignature(raw, acct.Sqid, fx.srv.billing.cfg.PriceBook, "IE", form.Get("quoted_currency"), form.Get("currency_chosen") == "1", 1)) |
| + form.Set("quote_sig", fx.srv.billingQuoteSignature(raw, acct.Sqid, fx.srv.billing.cfg.PriceBook, "IE", plans.BandA, form.Get("quoted_currency"), form.Get("currency_chosen") == "1", 1)) |
| r := httptest.NewRequest(http.MethodPost, fx.path("/billing/checkout"), strings.NewReader(form.Encode())) |
| r.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| r.AddCookie(&http.Cookie{Name: sessionCookie, Value: raw}) |
| @@ -2013,7 +2013,7 @@ func TestBillingFormPostsWhatItShows(t *testing.T) { |
| // form discloses: refused, because the key is the server's secret. |
| _, fields = b.page("?country=IN¤cy=gbp") |
| disclosed := hmac.New(sha256.New, []byte(fields.Get("csrf"))) |
| - fmt.Fprintf(disclosed, "%s\x00%s\x00%s\x00%s\x00%s\x00%s\x00%s", fields.Get("csrf"), fx.sqid, fx.srv.billing.cfg.PriceBook, "IN", "usd", "0", fields.Get("quoted_members")) |
| + fmt.Fprintf(disclosed, "%s\x00%s\x00%s\x00%s\x00%s\x00%s\x00%s\x00%s", fields.Get("csrf"), fx.sqid, fx.srv.billing.cfg.PriceBook, "IN", plans.BandD, "usd", "0", fields.Get("quoted_members")) |
| fields.Set("currency", "usd") |
| fields.Set("currency_chosen", "") |
| fields.Set("quoted_currency", "usd") |
| @@ -2072,6 +2072,19 @@ func TestBillingFormPostsWhatItShows(t *testing.T) { |
| t.Fatalf("intent %+v %v; want twelve members", intent, err) |
| } |
| }) |
| + t.Run("a band changed by the operator between the page and the post is a new quote", func(t *testing.T) { |
| + fx, _, acct := newCurrencyFixture(t) |
| + b := newBillingBrowser(t, fx) |
| + _, fields := b.page("?country=IN¤cy=gbp") |
| + fields.Set("postal", "110001") |
| + fx.srv.billing.cfg.CountryBands["IN"] = plans.BandB |
| + if w := b.submit(fields); w.Code != http.StatusSeeOther || !strings.HasPrefix(w.Header().Get("Location"), fx.path("/billing")+"?country=IN¤cy=gbp") { |
| + t.Fatalf("a form priced at band D posted after India moved to band B: %d %s", w.Code, w.Header().Get("Location")) |
| + } |
| + if _, err := fx.st.billingCheckoutIntent(acct.ID); err == nil { |
| + t.Fatal("an intent was opened at a band the page did not price") |
| + } |
| + }) |
| t.Run("a currency that is not sold is refused before anything", func(t *testing.T) { |
| fx, _, acct := newCurrencyFixture(t) |
| b := newBillingBrowser(t, fx) |
| @@ -2833,9 +2846,32 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_quantity_events WHERE account_id=? AND intent_id=? AND processed_at=0`, acct.ID, intent.ID).Scan(&unprocessed); err != nil || unprocessed != 0 { |
| t.Fatalf("%d quantity events of the retired intent left unprocessed (%v)", unprocessed, err) |
| } |
| + // And the settled events hold nothing: the account is not obliged, a |
| + // month freezes, and a second start of the console marks no history |
| + // gap. |
| + tx, err := fx.st.db.Begin() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + held, err := quantityObligationTx(tx, acct.ID) |
| + tx.Rollback() |
| + if err != nil || held { |
| + t.Fatalf("after the resolution the account is held (%v %v); nothing is open to clear it", held, err) |
| + } |
| + if _, err := fx.st.freezeBillingMonth(acct.ID, "2026-09", []byte(`{"statement_object":{"key":""},"v":2,"arithmetic":"base_units_floor_v1","currency":"gbp","statement":{}}`)); err != nil { |
| + t.Fatalf("a month after the resolution: %v", err) |
| + } |
| + if err := fx.st.migrateBilling(); err != nil { |
| + t.Fatal(err) |
| + } |
| + var gaps int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_quantity_changes WHERE account_id=? AND history_gap<>0`, acct.ID).Scan(&gaps); err != nil || gaps != 0 { |
| + t.Fatalf("%d history gaps after a second start (%v)", gaps, err) |
| + } |
| // A new purchase opens a new intent; a binding of the old session that |
| // read it before the resolution files nothing now, and a resolution |
| - // named for the old intent touches the new one not at all. |
| + // named for the old intent (already resolved: it returns before any |
| + // delete) touches the new one not at all. |
| f.mu.Lock() |
| f.sessionStatus, f.sessionCurrency = "open", "" |
| f.mu.Unlock() |
| @@ -2870,3 +2906,89 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| t.Fatalf("state %+v, %d expiries; want nothing recorded or expired", state, f.expireCalls) |
| } |
| } |
| + |
| +// A refused, paid session found by the listing (the create response was |
| +// lost, the intent was never bound) is recorded by its id with the review |
| +// and in the retirement record: the operator does not have to find it at |
| +// the provider by its metadata. |
| +func TestUnboundRefusedSessionIsRecordedByItsId(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry, f.failCreate = "GB", true |
| + if _, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err == nil { |
| + t.Fatal("fixture: the create response was not lost") |
| + } |
| + intent, err := fx.st.billingCheckoutIntent(acct.ID) |
| + if err != nil || intent.SessionID != "" { |
| + t.Fatalf("fixture: intent %+v %v", intent, err) |
| + } |
| + f.mu.Lock() |
| + f.failCreate, f.sessionStatus, f.sessionCurrency, f.subscriptionPrice = false, "complete", "eur", intent.Price |
| + f.mu.Unlock() |
| + _ = fx.srv.billingPass(context.Background(), f.now.Add(time.Minute)) |
| + 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 != "cs_test_paid" { |
| + t.Fatalf("the review's reference: %q %v; want the refused session's id", reference, err) |
| + } |
| + if after, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || after.SessionID != "" { |
| + t.Fatalf("the intent was bound to the refused session: %+v %v", after, 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: "refunded"}, f.now.Add(time.Hour)); err != nil { |
| + t.Fatal(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 != "cs_test_paid" { |
| + t.Fatalf("the retirement record's session: %q %v", retired, err) |
| + } |
| +} |
| + |
| +// The pass's ownership test protects the binder's own second read as |
| +// well: a first answer that is the intent's own, then a second that names |
| +// another account's everything under the same id, binds, reviews and |
| +// records nothing for either account. |
| +func TestBindersSecondReadIsJudgedAgainstTheIntent(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("A's click: dest %q err %v", dest, err) |
| + } |
| + intentA, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + if err := fx.st.CreateAccount(acct.ID+100, "cdef", "Other", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + other, _ := fx.st.AccountBySqid("cdef") |
| + f.mu.Lock() |
| + f.failCreate = true |
| + f.mu.Unlock() |
| + if _, err := fx.srv.startBillingCheckout(context.Background(), other, plans.PlanPersonal, "GB", "", "", f.now); err == nil { |
| + t.Fatal("fixture: B's create response was not lost") |
| + } |
| + intentB, _ := fx.st.billingCheckoutIntent(other.ID) |
| + f.mu.Lock() |
| + f.failCreate, f.sessionStatus, f.subscriptionPrice = false, "complete", intentA.Price |
| + // The first rendering is A's own; the second is B's in everything but |
| + // the id. |
| + f.sessionAnswers = []func(*fakeBillingStripe){ |
| + func(*fakeBillingStripe) {}, |
| + func(f *fakeBillingStripe) { |
| + f.sessionSqid = "cdef" |
| + f.meta["checkout_intent"], f.meta["account_sqid"] = intentB.ID, "cdef" |
| + }, |
| + } |
| + f.mu.Unlock() |
| + if err := fx.srv.reconcilePendingIntent(context.Background(), intentA, f.now.Add(time.Minute)); err == nil { |
| + t.Fatal("the pass accepted a second read that was B's") |
| + } |
| + stateB, _ := fx.st.billingAccount(other.ID) |
| + afterB, _ := fx.st.billingCheckoutIntent(other.ID) |
| + stateA, _ := fx.st.billingAccount(acct.ID) |
| + var reviewsEither int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_reviews WHERE reason=?`, billingCheckoutRefusedReason).Scan(&reviewsEither); err != nil { |
| + t.Fatal(err) |
| + } |
| + if stateB.Subscription != "" || afterB.SessionID != "" || stateA.Subscription != "" || f.expireCalls != 0 || reviewsEither != 0 { |
| + t.Fatalf("B %+v / %+v, A %+v, %d expiries, %d reviews; want nothing done for either account", stateB, afterB, stateA, f.expireCalls, reviewsEither) |
| + } |
| +} |
diff --git a/internal/console/billinghandlers.go b/internal/console/billinghandlers.go| index 6c403c47e..1a3467f46 100644 |
| --- a/internal/console/billinghandlers.go |
| +++ b/internal/console/billinghandlers.go |
| @@ -91,7 +91,15 @@ func (s *Server) handleBillingCheckout(w http.ResponseWriter, r *http.Request) { |
| // agree with each other buy nothing unseen. |
| quotedCountry, quotedCurrency, quotedChosen := r.PostForm.Get("quoted_country"), r.PostForm.Get("quoted_currency"), r.PostForm.Get("currency_chosen") == "1" |
| quotedMembers, merr := strconv.Atoi(r.PostForm.Get("quoted_members")) |
| - if merr != nil || country != quotedCountry || effective != quotedCurrency || !s.billingQuoteSigned(r.PostForm.Get("csrf"), acct.Sqid, s.billing.cfg.PriceBook, quotedCountry, quotedCurrency, quotedChosen, quotedMembers, r.PostForm.Get("quote_sig")) { |
| + // The band is a price input too: the deployment's band for the posted |
| + // country now must be the one the page priced at. |
| + band := "" |
| + if country != "" { |
| + if b, err := plans.BandForCountry(s.billing.cfg.CountryBands, country); err == nil { |
| + band = b |
| + } |
| + } |
| + if merr != nil || country != quotedCountry || effective != quotedCurrency || !s.billingQuoteSigned(r.PostForm.Get("csrf"), acct.Sqid, s.billing.cfg.PriceBook, quotedCountry, band, quotedCurrency, quotedChosen, quotedMembers, r.PostForm.Get("quote_sig")) { |
| http.Redirect(w, r, back+billingQuoteQuery(country, currency, chosen), http.StatusSeeOther) |
| return |
| } |
diff --git a/internal/console/billingpage.go b/internal/console/billingpage.go| index 16bc91be8..8887eba0b 100644 |
| --- a/internal/console/billingpage.go |
| +++ b/internal/console/billingpage.go |
| @@ -264,7 +264,11 @@ func (s *Server) handleBillingPage(w http.ResponseWriter, r *http.Request) { |
| } |
| terms := s.pageCurrency(r, v, country, state) |
| v.Plans = billingPlanRows(terms, band, int64(v.Members)) |
| - v.QuoteSig = s.billingQuoteSignature(p.CSRF, acct.Sqid, terms.PriceBook, v.QuotedCountry, v.Currency, v.CurrencyChosen, v.Members) |
| + quotedBand := "" |
| + if v.QuotedCountry != "" { |
| + quotedBand = band |
| + } |
| + v.QuoteSig = s.billingQuoteSignature(p.CSRF, acct.Sqid, terms.PriceBook, v.QuotedCountry, quotedBand, v.Currency, v.CurrencyChosen, v.Members) |
| } |
| p.Billing = v |
| @@ -429,15 +433,17 @@ func billingPlanRows(terms plans.PriceTerms, band string, members int64) []billi |
| return rows |
| } |
| -// billingQuoteSignature is the page's word that it priced country in |
| -// currency (chosen or a default) under book, for the session whose token |
| -// is sessionToken, the account sqid, and members members (the quantity a |
| -// team plan is priced at follows from it): HMAC-SHA256 over those, hex, |
| -// keyed by a secret derived from the billing secret key, which only the |
| -// server holds. The form discloses the session token (as csrf) and every |
| -// quoted field, and none of them is the key, so only a page this console |
| -// rendered can produce the signature for a quote. |
| -func (s *Server) billingQuoteSignature(sessionToken, sqid, book, country, currency string, chosen bool, members int) string { |
| +// billingQuoteSignature is the page's word that it priced country, at |
| +// band (the deployment's band for it at render time, "" with no country), |
| +// in currency (chosen or a default) under book, for the session whose |
| +// token is sessionToken, the account sqid, and members members (the |
| +// quantity a team plan is priced at follows from it): HMAC-SHA256 over |
| +// those, hex, keyed by a secret derived from the billing secret key, which |
| +// only the server holds. The form discloses the session token (as csrf) |
| +// and every quoted field, and none of them is the key, so only a page this |
| +// console rendered can produce the signature for a quote; and a band map |
| +// changed by the operator between render and post fails it. |
| +func (s *Server) billingQuoteSignature(sessionToken, sqid, book, country, band, currency string, chosen bool, members int) string { |
| root := hmac.New(sha256.New, []byte(s.billing.cfg.SecretKey)) |
| root.Write([]byte("carloku/billing/quote/v1")) |
| mac := hmac.New(sha256.New, root.Sum(nil)) |
| @@ -445,13 +451,13 @@ func (s *Server) billingQuoteSignature(sessionToken, sqid, book, country, curren |
| if chosen { |
| flag = "1" |
| } |
| - fmt.Fprintf(mac, "%s\x00%s\x00%s\x00%s\x00%s\x00%s\x00%d", sessionToken, sqid, book, country, currency, flag, members) |
| + fmt.Fprintf(mac, "%s\x00%s\x00%s\x00%s\x00%s\x00%s\x00%s\x00%d", sessionToken, sqid, book, country, band, currency, flag, members) |
| return hex.EncodeToString(mac.Sum(nil)) |
| } |
| // billingQuoteSigned reports whether sig is the signature of that quote. |
| -func (s *Server) billingQuoteSigned(sessionToken, sqid, book, country, currency string, chosen bool, members int, sig string) bool { |
| - return hmac.Equal([]byte(s.billingQuoteSignature(sessionToken, sqid, book, country, currency, chosen, members)), []byte(sig)) |
| +func (s *Server) billingQuoteSigned(sessionToken, sqid, book, country, band, currency string, chosen bool, members int, sig string) bool { |
| + return hmac.Equal([]byte(s.billingQuoteSignature(sessionToken, sqid, book, country, band, currency, chosen, members)), []byte(sig)) |
| } |
| // formatFigure renders num/den as the approved sentences write a figure: |
diff --git a/internal/console/billingpass.go b/internal/console/billingpass.go| index 447e0d140..d60de3f64 100644 |
| --- a/internal/console/billingpass.go |
| +++ b/internal/console/billingpass.go |
| @@ -197,7 +197,7 @@ func (s *Server) reconcilePendingIntent(ctx context.Context, intent billingInten |
| } |
| switch session.Status { |
| case "complete": |
| - id, err := s.bindBillingCheckout(ctx, session.ID) |
| + id, err := s.bindBillingCheckoutFor(ctx, session.ID, &intent) |
| if err != nil { |
| return err |
| } |
| @@ -256,7 +256,7 @@ func (s *Server) reconcilePendingIntent(ctx context.Context, intent billingInten |
| if session.Status != "complete" { |
| return nil |
| } |
| - id, err := s.bindBillingCheckout(ctx, session.ID) |
| + id, err := s.bindBillingCheckoutFor(ctx, session.ID, &intent) |
| if err != nil { |
| return err |
| } |
diff --git a/internal/console/billingquantityaudit.go b/internal/console/billingquantityaudit.go| index 318186e42..9989ffa99 100644 |
| --- a/internal/console/billingquantityaudit.go |
| +++ b/internal/console/billingquantityaudit.go |
| @@ -17,9 +17,9 @@ func (s *Store) migrateQuantityHistoryGuards() error { |
| // this one, and only then, in one immediate transaction: two console |
| // processes opening the same database cannot meet between the drop and |
| // the create, and a database that is already current is not touched. |
| - const processingGuard = `CREATE TRIGGER billing_quantity_events_processing_guard BEFORE UPDATE ON billing_quantity_events |
| + processingGuard := `CREATE TRIGGER billing_quantity_events_processing_guard BEFORE UPDATE ON billing_quantity_events |
| WHEN NEW.processed_at<=0 OR NEW.processed_at<OLD.occurred_at_ns/1000000000 |
| - OR NEW.processing_outcome NOT IN ('charged','included','landed_unverified','checkout_expired','checkout_abandoned','checkout_refused_resolved') |
| + OR NEW.processing_outcome NOT IN ` + quantityOutcomesSQL + ` |
| OR length(trim(NEW.processing_reference))=0 OR length(NEW.processing_reference)>256 |
| BEGIN SELECT RAISE(ABORT,'billing_quantity_processing_invalid'); END` |
| if _, err := s.db.Exec(`CREATE TRIGGER IF NOT EXISTS billing_quantity_events_insert_guard BEFORE INSERT ON billing_quantity_events |
| @@ -40,15 +40,22 @@ func (s *Store) migrateQuantityHistoryGuards() error { |
| } |
| } |
| if _, err := s.db.Exec(`UPDATE billing_quantity_changes SET history_gap=1,state='history_unproven' |
| - WHERE EXISTS(SELECT 1 FROM billing_quantity_events e WHERE e.account_id=billing_quantity_changes.account_id AND e.processed_at<>0 AND (e.processed_at<e.occurred_at_ns/1000000000 OR e.processing_outcome NOT IN ('charged','included','landed_unverified','checkout_expired','checkout_abandoned') OR length(trim(e.processing_reference))=0 OR length(e.processing_reference)>256))`); err != nil { |
| + WHERE EXISTS(SELECT 1 FROM billing_quantity_events e WHERE e.account_id=billing_quantity_changes.account_id AND e.processed_at<>0 AND (e.processed_at<e.occurred_at_ns/1000000000 OR e.processing_outcome NOT IN ` + quantityOutcomesSQL + ` OR length(trim(e.processing_reference))=0 OR length(e.processing_reference)>256))`); err != nil { |
| return err |
| } |
| return nil |
| } |
| +// quantityOutcomesSQL is the one list of the outcomes a processed quantity |
| +// event may carry, as an SQL list, used by the processing guard, the |
| +// startup sweep and the obligation check alike: an outcome admitted by one |
| +// and unknown to another would hold an account for good with nothing open |
| +// to clear it (review seven of the currency branch found exactly that). |
| +const quantityOutcomesSQL = "('charged','included','landed_unverified','checkout_expired','checkout_abandoned','checkout_refused_resolved')" |
| + |
| func quantityObligationTx(tx *sql.Tx, accountID int64) (bool, error) { |
| var held bool |
| - err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM billing_quantity_events WHERE account_id=? AND ((change='addition' AND processed_at=0) OR processed_at<0 OR (processed_at<>0 AND (processed_at<occurred_at_ns/1000000000 OR processing_outcome NOT IN ('charged','included','landed_unverified','checkout_expired','checkout_abandoned') OR length(trim(processing_reference))=0 OR length(processing_reference)>256)))) |
| + err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM billing_quantity_events WHERE account_id=? AND ((change='addition' AND processed_at=0) OR processed_at<0 OR (processed_at<>0 AND (processed_at<occurred_at_ns/1000000000 OR processing_outcome NOT IN `+quantityOutcomesSQL+` OR length(trim(processing_reference))=0 OR length(processing_reference)>256)))) |
| OR EXISTS(SELECT 1 FROM billing_quantity_changes WHERE account_id=? AND history_gap<>0)`, accountID, accountID).Scan(&held) |
| return held, err |
| } |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index a3d7e9d0e..be9dd9726 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -904,6 +904,17 @@ func billingPeriodReviewReason(periodStart int64) string { |
| } |
| func (s *Server) bindBillingCheckout(ctx context.Context, sessionID string) (string, error) { |
| + return s.bindBillingCheckoutFor(ctx, sessionID, nil) |
| +} |
| + |
| +// bindBillingCheckoutFor is bindBillingCheckout for a caller working on a |
| +// 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. |
| +func (s *Server) bindBillingCheckoutFor(ctx context.Context, sessionID string, expected *billingIntent) (string, error) { |
| b := s.billing |
| if b == nil { |
| return "", errBillingUnavailable |
| @@ -915,6 +926,15 @@ func (s *Server) bindBillingCheckout(ctx context.Context, sessionID string) (str |
| if session.ID != sessionID || session.Status != "complete" || session.LiveMode != b.cfg.LiveMode || session.SubscriptionID == "" { |
| return "", errBillingPending |
| } |
| + if expected != nil { |
| + owner, err := s.store.AccountByID(expected.AccountID) |
| + if err != nil { |
| + return "", err |
| + } |
| + if owner == nil || !b.ownSession(session, *expected, owner) || session.Metadata["account_sqid"] != owner.Sqid { |
| + return "", errBillingIdentity |
| + } |
| + } |
| acct, err := s.store.AccountBySqid(session.Metadata["account_sqid"]) |
| if err != nil { |
| return "", err |
diff --git a/internal/console/billingservice_test.go b/internal/console/billingservice_test.go| index 15eaf3ccf..49d940069 100644 |
| --- a/internal/console/billingservice_test.go |
| +++ b/internal/console/billingservice_test.go |
| @@ -51,6 +51,10 @@ type fakeBillingStripe struct { |
| // sessionSqid, when set, is the client_reference_id the session names |
| // in place of the fixture account's. |
| sessionSqid 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. |
| + sessionAnswers []func(*fakeBillingStripe) |
| // prices are extra price objects, by id, beside price_personal. |
| prices map[string]map[string]any |
| subscriptionPrice, subscriptionCurrency string |
| @@ -127,6 +131,11 @@ func newBillingFixture(t *testing.T) (*consoleFixture, *fakeBillingStripe, *Acco |
| } |
| func (f *fakeBillingStripe) session() map[string]any { |
| + if len(f.sessionAnswers) > 0 { |
| + next := f.sessionAnswers[0] |
| + f.sessionAnswers = f.sessionAnswers[1:] |
| + next(f) |
| + } |
| sub := "" |
| customer := "cus_paid" |
| payment := "unpaid" |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index bd319f195..991455ca0 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -199,12 +199,12 @@ func (s *Store) migrateBilling() error { |
| // version 1. The version is read with json_extract, not by |
| // substring. Earlier builds of this branch created a wider trigger |
| // under the name billing_months_no_legacy_calc_over_book_history; |
| - // it is dropped first, so a database they opened gets this one. |
| - // (This CREATE runs first; the DROP of the earlier name runs after |
| - // it, in a transaction that first confirms this one exists |
| - // (retireEarlierBarrierTrigger), so no moment leaves the database |
| - // with neither: the ordering is the guarantee, not a shared |
| - // transaction.) |
| + // it is dropped after this one is created, so a database they |
| + // opened gets this one and no moment has neither. (This CREATE |
| + // runs first; the DROP of the earlier name runs after it, in a |
| + // transaction that first confirms this one exists |
| + // (retireEarlierBarrierTrigger): the ordering is the guarantee, |
| + // not a shared transaction.) |
| `CREATE TRIGGER IF NOT EXISTS billing_months_no_legacy_calc_over_paid_book_history BEFORE INSERT ON billing_months |
| WHEN (CASE WHEN json_valid(CAST(NEW.calculation AS TEXT)) THEN json_extract(CAST(NEW.calculation AS TEXT), '$.v') IS NOT 2 ELSE 1 END) |
| AND EXISTS (SELECT 1 FROM billing_book_assignments b WHERE b.account_id = NEW.account_id |
| @@ -237,6 +237,11 @@ func (s *Store) migrateBilling() error { |
| // Existing rows, open or resolved, have been filed at least once; their |
| // resolved_at is left exactly as it was. last_seen_at starts from |
| // created_at, the only observation such a row recorded. |
| + // reference: what a review is about beyond its key, when the key does |
| + // not say (a refused session's id, for a review keyed by its intent). |
| + if err := s.addColumnIfMissing("billing_reviews", "reference", `TEXT NOT NULL DEFAULT ''`); err != nil { |
| + return err |
| + } |
| if err := s.addColumnIfMissing("billing_reviews", "occurrences", `INTEGER NOT NULL DEFAULT 1`); err != nil { |
| return err |
| } |
| @@ -915,6 +920,15 @@ func (s *Store) resolveBillingReview(r billingReviewResolution, now time.Time) e |
| return err |
| } |
| if err == nil { |
| + // The refused session: the one the intent was bound to, or, |
| + // for an intent that was never bound (the create response was |
| + // lost and the listing found the session paid), the one the |
| + // review was filed about. |
| + if sessionID == "" { |
| + if rerr := tx.QueryRow(`SELECT reference FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=?`, r.AccountID, r.Subscription, r.Reason).Scan(&sessionID); rerr != nil && !errors.Is(rerr, sql.ErrNoRows) { |
| + return rerr |
| + } |
| + } |
| reference := sessionID |
| if reference == "" { |
| reference = "checkout_refused_resolved" |
| @@ -938,8 +952,10 @@ func (s *Store) resolveBillingReview(r billingReviewResolution, now time.Time) e |
| // in a currency or on price terms other than its intent asked for: once |
| // while open, keyed by the intent (the review's subscription field holds |
| // the intent's id, which is what the operator's resolution then retires), |
| -// and only while that intent still stands, so a binding that read the |
| -// session before the resolution and files after it files nothing. |
| +// with the session's id as the review's reference (the intent may never |
| +// have been bound to it), and only while that intent still stands, so a |
| +// binding that read the session before the resolution and files after it |
| +// files nothing. |
| func (s *Store) fileCheckoutRefusedReview(intent billingIntent, sessionID string, observed time.Time) error { |
| tx, err := s.db.Begin() |
| if err != nil { |
| @@ -963,6 +979,9 @@ func (s *Store) fileCheckoutRefusedReview(intent billingIntent, sessionID string |
| if err := fileBillingReviewTx(tx, intent.AccountID, intent.ID, billingCheckoutRefusedReason, observed); err != nil { |
| return err |
| } |
| + if _, err := tx.Exec(`UPDATE billing_reviews SET reference=? WHERE account_id=? AND subscription=? AND reason=? AND reference=''`, sessionID, intent.AccountID, intent.ID, billingCheckoutRefusedReason); err != nil { |
| + return err |
| + } |
| return tx.Commit() |
| } |