amadan
Multi-currency: the ninth review's findings
The startup sweep no longer clears any quantity-history gap: a gap's cause is not recorded, so a sweep that cleared marks it took to be its own would have cleared the pre-history and clock-rollback holds too; the roll note forbids the rollback that would make an obsolete-outcome mark, and clearing one is an operator's door that does not exist yet. The two holds now survive a restart in their tests. A session offered on resume must match the intent's plan, band and terms as well as its ownership and price terms, so an open session with one of them altered is refused and expired rather than paid and reviewed. Six stale sentences in the dispositions are corrected.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8 files changed,
+291
−53
docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review9.md+110 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+20 −6docs/superpowers/specs/OPUS-REVIEW-multi-currency-review9.md+103 −0internal/console/billingcurrency_test.go+33 −35internal/console/billingmembership_test.go+8 −0internal/console/billingquantityaudit.go+7 −12internal/console/billingquantityaudit_test.go+7 −0internal/console/billingservice.go+3 −0
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review9.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review9.md| new file mode 100644 |
| index 000000000..ffa6fa0ff |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review9.md |
| @@ -0,0 +1,110 @@ |
| +| Eighth-review item | Closed at `f8a23ef3`? | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **Opus P3-1 — rollback leaves a permanent quantity-history hold** | The reported case is repaired, but the repair clears unrelated holds too: **new P2 below**. | **No:** `internal/console/billingcurrency_test.go:3116` requires healing. However, the test passes with the unsafe clearing of other causes. | |
| +| **Opus P3-2 — refused session ID unavailable to the operator** | **Yes**, through the review listing and API: `internal/console/billingoperator.go:61`, `:179`, `:198`. | **No:** the actual API response is asserted at `internal/console/billingcurrency_test.go:2984`; the listing reader is checked at `:3074`. | |
| +| **Opus’s claims** | **Partly corrected.** Membership-lock wording, the two-account fixture comment and negative ownership-webhook status assertions are corrected. Several disposition and mutation claims remain inaccurate below. | The migration test still permits unsafe ordering; the repeated-resolution test still permits removing the DELETE’s intent predicate. | |
| +| **Astra 1 — terms mismatch bypasses the completed-session review** | **Yes:** ownership precedes the combined purchase-identity check at `internal/console/billingservice.go:964`; mismatches reach the review at `:978`. | **No:** independent pass and fresh-webhook cases change both terms and currency at `internal/console/billingcurrency_test.go:2369`. Restoring the early rejection leaves no review. | |
| +| **Astra 2 — reference missing from the operator API** | **Yes**, as for Opus P3-2. The singular `billingReview` reader remains incomplete, qualified below. | **No:** the API assertion distinguishes its omission. | |
| +| **Astra 3 — existing open review never learns its reference** | **Yes:** `internal/console/billingstore.go:980` fills an empty reference whether the review is new or already open. | **No:** `internal/console/billingcurrency_test.go:2991` blanks the reference, reobserves the session and requires its restoration with one occurrence. | |
| +| **Astra’s claims** | **Partly corrected.** The runtime refusal and operator-visibility defects are fixed; blanket coverage, mutation and historical-review claims remain overstated. | Source establishes the distinguishing cases above, not the claimed historical mutation runs. | |
| + |
| +**Severity disagreement:** Astra’s eighth-round terms-mismatch finding was supported: an owned completed payment could miss the account review needed for settlement and retirement. Opus’s “no P2” conclusion overlooked that variant. For the carried-forward second-read disagreement, Opus correctly identified the prerequisite— inconsistent authenticated provider responses, without a demonstrated buyer exploit. Astra correctly identified the consequence: processing A’s intent could bind B. **P2 is appropriate for that ownership failure**, with the prerequisite stated explicitly. |
| + |
| +All platform references below are at `f8a23ef3`; pricing-site references are at `9427c22`. The requested `git diff f8a23ef3 f8a23ef3` is empty. This was source inspection against `12e70d95`, with no modifications, tests, network commands or AWS commands. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P2 — Startup can erase a genuine missing-history hold.** |
| + |
| + **Where:** `internal/console/billingquantityaudit.go:53`. Other causes of the same state are established at `internal/console/billingmembership.go:143` and `:466`. |
| + |
| + **Concrete scenario:** |
| + |
| + - A team’s quantity summary predates membership-event recording. A member added and removed during that period left no event. |
| + - Migration deliberately preserves `history_gap=1`; a subsequent summary write assigns `state='history_unproven'`. |
| + - On the next startup, the new clearing statement finds no invalid *recorded processed events*. This also succeeds when there are no events. |
| + - It clears `history_gap`, although the missing membership history has not been recovered. |
| + - `quantityObligationTx` at `internal/console/billingquantityaudit.go:70` now releases that hold. A month can pass the quantity-history gate at `internal/console/billingstore.go:1105` and freeze without the historical evidence that previously prevented it. |
| + |
| + Clock-reversal gaps use the same state and are also eligible. `history_unproven` does not identify the cause of a gap. |
| + |
| + **Smallest acceptable fix:** Remove automatic clearing and retain the rollback prohibition. If automatic repair is required, persist independent gap causes and clear only a positively identified obsolete-outcome cause. Existing ambiguous gaps must remain held. |
| + |
| + **Regression coverage:** Extend `TestQuantityHistoryMigrationMarksOldSummaryUnproven` at `internal/console/billingmembership_test.go:667` with another migration after the state becomes `history_unproven`, then assert the obligation and freeze remain blocked. Add a clock-reversal case. The new test at `internal/console/billingcurrency_test.go:3096` distinguishes removing healing, but does not distinguish safe healing from this implementation. |
| + |
| +**Remaining requested paths** |
| + |
| +- **A — GBP purchase end to end.** `GET ?country=IN¤cy=gbp` produces India’s band D and an explicit GBP quote, including Personal at £7.70 (`internal/console/billingpage.go:265`). The posted form’s effective currency and signed quote are checked at `internal/console/billinghandlers.go:81` and `:102`. Membership is checked before the intent snapshots the purchase (`internal/console/billingservice.go:410`, `:522`). |
| + |
| + `checkoutParams` carries that currency; `CreateCheckout` sends it with Adaptive Pricing disabled (`internal/console/billingservice.go:229`; `internal/billingstripe/client.go:622`). Binding checks ownership and purchase identity (`internal/console/billingservice.go:964`); reconciliation checks subscription/item currency and the completed session (`:748`, `:790`). Account and assignment writes preserve the book and currency (`internal/console/billingstore.go:594`, `:725`). |
| + |
| + History reads both assignment tables with those fields (`internal/console/billinginvoicestore.go:530`). `AllowanceForMonth` requires one paid commercial identity; `priceBillingCalculation` creates a GBP version-2 snapshot (`internal/console/billingcalculation.go:174`). Drafts, items, recovery, final verification and request hashes retain GBP (`internal/console/billinginvoice.go:859`, `:885`, `:907`, `:961`, `:1021`, `:1113`). The quantity worker selects the stored price and verifies the returned price/currency (`internal/console/billingquantity.go:1016`, `:1105`). I found no conversion or alternate-currency pricing path in this chain. |
| + |
| +- **B/M/Q/U/Z — One form and quote integrity.** The HMAC key is derived from the server’s billing secret, with a separate label, and covers session token, account, book, country, band, currency, choice flag and member count (`internal/console/billingpage.go:446`). Disclosed form values cannot produce another valid signature. Reuse works for the identical tuple; another session, account, book, band or count fails. Token rotation also invalidates CSRF. |
| + |
| + Changed country/currency, forged agreeing `quoted_*` values or a changed choice flag cannot buy an unseen quote (`internal/console/billinghandlers.go:102`). A quote in another tab does not invalidate the first tab’s independently signed quote; the first remains valid only for its own inputs and current membership. |
| + |
| + Present-empty, repeated, uppercase or whitespace-padded currency fields are refused on both doors (`internal/console/billinghandlers.go:176`). With no currency field, the effective purchase uses the country default and must still match the authenticated quote. This also preserves the legacy form’s behavior. Plan is omitted from the signature because the page displays all offered plan prices; quantity follows authenticated membership and plan constraints. Postal affects tax subsequently presented by Stripe. |
| + |
| + The quoted count is compared under the billing lock, then the intent snapshot is checked under the membership transaction (`internal/console/billingservice.go:410`; `internal/console/billingstore.go:377`). A changed count conflicts. |
| + |
| +- **B/C/O — Quote door, suggestions and held currency.** The quote door checks session, form token and membership, then builds an escaped account billing URL from posted values (`internal/console/billinghandlers.go:137`, `:197`). It cannot redirect to a supplied destination or create a purchase. |
| + |
| + Present-empty parameters clear cookies; absent parameters fall back; invalid parameters set nothing (`internal/console/billingcurrency.go:43`). An invalid parameter leaves an old cookie intact, so a later request with that parameter absent can show the old suggestion again. That follows the specified “invalid changes nothing” behavior. Cookies never directly choose checkout currency. |
| + |
| + A held currency is offered alone, including EUR for pre-book purchases, regardless of URL or cookie (`internal/console/billingpage.go:301`). The service separately refuses another currency (`internal/console/billingservice.go:510`). |
| + |
| +- **D/L/R/W/Y/AA — Ownership and binding.** `ownSession` checks mode, intent metadata, client reference, known customer and bound session ID (`internal/console/billingservice.go:259`). Expected-intent binding also checks response `account_sqid` against the expected account before deriving an account from the response (`:930`). Both pass binding calls carry their expected intent. Webhook binding establishes the response account, checks client reference, then checks its current intent. |
| + |
| + The binder now checks ownership before plan, band, terms, book and currency; an owned completed mismatch files the refusal review (`:964`). |
| + |
| + Two qualifications to “every path calls `ownSession`”: the already-recorded-subscription shortcut changes no binding and is followed by independent reconciliation; expiry responses use equivalent transactional checks. Expiry ID checks occur at `:303`/`:575`; retirement checks mode, intent, unpaid/expired state and then client reference/customer at `internal/console/billingstore.go:442` and `:465`. A failure rolls back the preceding DELETE. I found no foreign-session retirement through that ordering. |
| + |
| +- **S/V/X/Z/BB — Review and retirement.** Filing checks that the exact intent still exists inside its transaction (`internal/console/billingstore.go:969`). Resolution selects that intent, closes its events, records retirement and deletes by account plus intent ID (`:922`, `:940`, `:943`, `:947`). A later intent is untouched; late filing cannot recreate the retired intent’s review. The session reference supplies the retirement record for an unbound intent. |
| + |
| + `quantityOutcomesSQL` supplies the processing guard, audits and obligation check (`internal/console/billingquantityaudit.go:66`). No remaining independent outcome whitelist was found. A changed guard definition is replaced at startup. The defect is the new gap-clearing predicate, not outcome-list divergence. |
| + |
| +- **E/H/N/T — Barrier and rollout.** The trigger reads top-level JSON `v` and intersects each active paid book assignment with the month, paid-through endpoint and next assignment across both tables (`internal/console/billingstore.go:208`). This matches `AllowanceForMonth` for valid histories (`internal/plans/month.go:140`). Same-second supersession contributes zero span; free/inactive rows contribute no paid span but can terminate the preceding one. |
| + |
| + `period_end` is read as `PaidThrough` (`internal/console/billinginvoicestore.go:542`). Renewal and quantity writers move the effective instant while retaining the subscription’s paid-through endpoint (`internal/console/billingstore.go:610`, `:642`, `:725`). I found neither a valid-history version-1 bypass nor an overbroad refusal. |
| + |
| + The new trigger is created before the old one is retired. The retirement transaction checks that the replacement exists before DROP; a missing replacement preserves the old trigger, and a second startup is harmless (`internal/console/billingstore.go:797`). A crash can temporarily leave both, not neither. The test proves final state, not crash safety. Older binaries remain subject to the documented rollout restrictions; the trigger is not general downgrade compatibility. |
| + |
| +- **F/G/J — Figures, book and export.** Inclusions divide `PriceFor`’s pooled allowance by its actual charged quantity (`internal/console/billingpage.go:427`). Current resources and factors yield exact per-member figures within three decimals, including team quantities above minimum. Unavailable Personal uses its supported quantity and remains unavailable. Band-A sentences have verbatim assertions at `internal/console/billingcurrency_test.go:2610`. |
| + |
| + Validation rejects malformed, duplicate and inconsistent book data; arithmetic checks overflow. Configuration requires the current complete catalog and permits complete historical catalogs with unique price IDs. Provisioning’s at-least-36-key and own-only `currency_options` checks agree with the console. The public export contains commercial, resource and country data, with no account or credential disclosure found. |
| + |
| +- **K — Pricing site.** At `9427c22`, overage formatting preserves micro-unit precision; inclusion scaling and exactness checks agree with the exported rules (`copy-review/pricing/build.mjs:48`, `:103`, `:160`). Numeric approved strings are checked against the export; country options carry exported bands/defaults (`:144`, `:208`). Links always carry both parameters and forward currency only when explicitly chosen (`src/site.js:75`). |
| + |
| + The first-line draft marker, exact `PRICEBOOK_ALLOW_DRAFT=1` exception and ordinary-build check are present. The site’s copied book is byte-identical to this platform export. Locale-only display and an account’s held currency can intentionally differ from the eventual console suggestion; neither silently becomes an explicit purchase choice. |
| + |
| +**Claims the code contradicts** |
| + |
| +| Claim | What the pinned source establishes | |
| +|---|---| |
| +| The startup inverse clears a gap “this sweep itself marked” (`internal/console/billingquantityaudit.go:46`) | The state also represents missing historical events and clock reversal. This is the P2 above. | |
| +| Every review reader carries the reference | The supported listing/API do; singular `billingReview` still omits it at `internal/console/billingstore.go:786`. Its current consumers do not establish another operator-visibility failure. | |
| +| Provisioning tests read “generated checks” (`DISPOSITIONS-multi-currency.md:79`, `:120`) | They inspect shell source, not generated or executed checks. | |
| +| The old barrier is “dropped first” (`DISPOSITIONS-multi-currency.md:116`) | The replacement is created first. | |
| +| Removing the inclusion assertion is a killed mutation (`DISPOSITIONS-multi-currency.md:143`) | Removing an assertion over already-exact data still passes. The data mutation described at `:135` is different. The withdrawal and old claim coexist. | |
| +| The reviewers agreed on “the one P2” (`DISPOSITIONS-multi-currency.md:176`) | Its own `:181` records Astra’s additional ownership P2. | |
| +| Both eighth reports closed every seventh finding (`DISPOSITIONS-multi-currency.md:194`) | Astra explicitly marked reference handling partly closed. | |
| +| Every fix has a distinguishing test | Repeated resolution returns before DELETE, so that case does not distinguish deleting by account alone. Uninterrupted migration does not distinguish unsafe DROP-before-CREATE. | |
| +| The site test establishes export provenance (`test/pricing-pricebook.test.mjs:32`) | It checks schema, currencies and a source-note pattern, not equality with the pinned platform export. The current bytes match independently. | |
| + |
| +Review three’s surviving default-as-explicit mutation is correctly qualified for accepted ordinary forms. Review four’s broader mutation/coverage language remains overstated. Historical mutation execution cannot be verified from source. I found no concrete wall-clock, ordering, temporary-root or shared-cookie-jar failure in the inspected additions. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +1. **Catalog:** Read all 36 prices with expanded currency options; verify amounts, metadata, recurrence and tax behavior. Exercise pagination beyond 100 objects and a failed later page. Verification must fail rather than report success. |
| +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 absence of conversion. Capture incompatible price/session-currency refusal responses. |
| +3. **Renewal and quantity:** Exercise both proration policies and renewal in each currency. Verify price identity, quantity, amounts and assignment effective dates. |
| +4. **Overages:** Exercise every meter, rounding boundary and currency-specific floor through creation, recovery, finalization and payment. Verify line currency, subtotal and automatic tax, including canceled subscriptions. |
| +5. **Recovery:** Lose responses around checkout and invoice operations; replay and recover by listing within and beyond retention windows. Confirm original identities, legacy request compatibility and no duplicate charges. |
| +6. **Refusals:** Exercise open-session expiry, completed refusals, concurrent pass/webhook observations and repeated/reordered delivery. Expect one actionable review, visible session identity, no paid-session activation or expiry, exact-intent retirement, and no reopened review after a late webhook. |
| +7. **After resolution:** Start another purchase and process its next metered month. Confirm retirement evidence and closed quantity events without a recurring hold. |
| +8. **Customer and discounts:** Establish currency constraints during subscriptions, after cancellation and with outstanding invoices; exercise fixed-amount discounts, the percentage promotion and house coupon. |
| + |
| +The missing-history restart regression requires local database testing separately; Stripe cannot establish that property. |
| + |
| +**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 538993814..d5c34df1d 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -76,7 +76,7 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## Review three (at de81dd70; Opus accepted with fixes, Astra rejected) |
| -Reports: `OPUS-REVIEW-multi-currency-review3.md`, `CODEX-FINDINGS-multi-currency-review3.md`. Both found the second review's findings closed and the GBP purchase carrying one currency end to end; Astra found the first resume guard not independently covered (below). The two disagreed on severity, not on the code: every finding of either is treated as a defect here, and each code change has a test that fails without it; the two changes to `stripe-provision.sh` had none until review four (its test now reads the generated checks). |
| +Reports: `OPUS-REVIEW-multi-currency-review3.md`, `CODEX-FINDINGS-multi-currency-review3.md`. Both found the second review's findings closed and the GBP purchase carrying one currency end to end; Astra found the first resume guard not independently covered (below). The two disagreed on severity, not on the code: every finding of either is treated as a defect here, and each code change has a test that fails without it; the two changes to `stripe-provision.sh` had none until review four (its test now reads the script's text for those checks). |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| @@ -113,11 +113,11 @@ Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency |
| | 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 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 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 at migration, after the new one is confirmed. | `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) | |
| | Opus P3-2, Astra 7 (P3): a purchase from before price books was not held. | Fixed: any account with a subscription is held in `state.priceTerms().Money()`, legacy EUR included. | `billingpage.go` | `TestBillingFormPostsWhatItShows` ("bought before price books is offered its euro alone") | |
| | Opus P3-4, Astra 8 (P3): the static site links carried neither parameter. | Fixed on the site (`4feb1cd`): every link carries `currency=&country=` statically; the script replaces the values. | site `build.mjs` | site `pricing-pricebook.test.mjs` | |
| -| Astra 9 (P3): the script's Check 2 demanded exactly 36 active prices in the account. | Fixed: it counts the book's own (metadata `price_book`); the plans test reads the generated checks. | `stripe-provision.sh`; `plans/provision_test.go` | `TestStripeProvisionScriptMatchesThePriceBook` | |
| +| Astra 9 (P3): the script's Check 2 demanded exactly 36 active prices in the account. | Fixed: it counts the book's own (metadata `price_book`); the plans test reads the script's text for them. | `stripe-provision.sh`; `plans/provision_test.go` | `TestStripeProvisionScriptMatchesThePriceBook` | |
| | Astra 10 (P3): `PRICEBOOK_ALLOW_DRAFT=0` allowed a draft. | Fixed on the site: exactly `"1"` in the build and the check; the preview is written before the refusal, as the comment said. | site | site test (unset, empty, `0`, `false`, `yes` refuse) | |
| | Opus P3-6: the draft gate holds only through `npm run check`. | Stated, not changed: the runbook runs `check`; a `build` hook is a site-build decision for the morning. | | | |
| | Opus P3-3: a resolved refused-paid review is reopened by the next pass while the intent stands. | Open: an operator's door that retires such an intent once the money is settled does not exist; the comment and the design say "once while open". | | | |
| @@ -140,7 +140,7 @@ Reports: `OPUS-REVIEW-multi-currency-review5.md`, `CODEX-FINDINGS-multi-currency |
| | Astra 2 (P2): quote fields forged to agree with each other bought a price no console page had shown. | Fixed in part here, keyed wrongly: the page signed its quote with the session's own token, which the form discloses, so the buyer could forge it (review six, both reviewers). Review six re-keys it with a server secret and covers the account and the member count. | `billingpage.go`, `billinghandlers.go`, `billing.html` | `TestBillingFormPostsWhatItShows` ("a choice flag erased": forged agreeing fields, a tampered signature and a missing one all redirect to the quote and open nothing; the rendered dollar quote buys; two sessions' signatures differ) | |
| | Astra 3 (P2), Opus P3-2: the trigger swap ran as two statements, so a crash between them left neither trigger. | Fixed: the new trigger is created in the migration list as before, and the old one is dropped afterwards in a transaction that first confirms the new one exists; no moment has neither (the ordering is the guarantee; the two do not share a transaction, and no test can show the atomicity itself, only the result). The roll note gains: builds of this branch from before the rename must not run after this one (they would create the old name beside the new). | `billingstore.go` `retireEarlierBarrierTrigger` | `TestBarrierTriggerLetsThisBuildsLegacyMonthsThrough` ("a database that carries the earlier, wider trigger") | |
| | 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-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 data mutant, a figure made not whole in base units, is killed; removing the assertion over exact data shows nothing) | |
| | 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 (review eight: the negative webhook cases now assert their status too, 204 for an acknowledged refusal and 503 for a read the console cannot yet settle). | 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; 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) | |
| @@ -173,7 +173,7 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## 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. |
| +Reports: `OPUS-REVIEW-multi-currency-review7.md`, `CODEX-FINDINGS-multi-currency-review7.md`. The two agree on every finding this round, and on the first two as P2 (the outcome list, and the binder's second read): 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 | |
| | --- | --- | --- | --- | |
| @@ -191,7 +191,7 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## Review eight (at 3e941f00; Opus accepted with fixes, Astra rejected) |
| -Reports: `OPUS-REVIEW-multi-currency-review8.md`, `CODEX-FINDINGS-multi-currency-review8.md`. Both found every seventh-round finding closed; neither found a P1 or a P2 in the code's money paths. Opus: a rollback trap the seventh round's outcome created, and the refused session's id not shown to the operator while the review is open. Astra: an owned completed session whose terms version changed with its currency was refused before the review branch (a variant of its round-four finding 6), the reference not carried through the operator API, and an already-open review from an earlier build never learning its reference. All fixed below, each with a test that fails without it; four mutants were killed in a scratch copy. |
| +Reports: `OPUS-REVIEW-multi-currency-review8.md`, `CODEX-FINDINGS-multi-currency-review8.md`. Both found every seventh-round finding closed; neither found a P1, and Astra's one P2 was in the binder's review routing, not in a money path. Opus: a rollback trap the seventh round's outcome created, and the refused session's id not shown to the operator while the review is open. Astra: an owned completed session whose terms version changed with its currency was refused before the review branch (a variant of its round-four finding 6), the reference not carried through the operator API, and an already-open review from an earlier build never learning its reference. All fixed below, each with a test that fails without it; four mutants were killed in a scratch copy. |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| @@ -206,3 +206,17 @@ Mutation checks in a scratch copy: a terms mismatch refused before ownership (no |
| 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. |
| + |
| +## Review nine (at f8a23ef3; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review9.md`, `CODEX-FINDINGS-multi-currency-review9.md`. Both found every eighth-round finding closed and agreed on the one P2: the startup sweep added in round eight to heal a gap marked under an obsolete outcome list could not tell such a mark from the pre-history default or the clock-rollback hold, which read the same (`history_unproven`), and would have cleared those on the next start. Fixed below with the tests both asked for; one P3 from Opus fixed too. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P2-1, Astra 1 (P2): the heal sweep cleared gaps set on purpose. | Fixed by removal: no sweep clears a gap; a gap's cause is not recorded. The roll note forbids the rollback that would make an obsolete-outcome mark; clearing one is an operator's door that does not exist yet (open). | `billingquantityaudit.go` | `TestQuantityHistoryMigrationMarksOldSummaryUnproven` and `TestQuantityAuditClockRollbackPreservesEventAndHolds` (each now restarts the store and asserts the gap stays); `TestAGapFromAnObsoleteOutcomeListStaysHeld` (the mark stays, the account stays held) | |
| +| Opus P3-1: `intentSession` did not judge plan, band or terms version, so an open session with one of them altered was offered and paid, then reviewed, where it could have been expired. | Fixed: `intentSession` compares plan, band and terms version too, so the resume paths refuse and expire such a session. | `billingservice.go` | `TestCheckoutNeverOffersASessionInAnotherCurrency` ("an open session whose plan, band or terms changed is refused and expired") | |
| +| Opus claims: six stale sentences in this file (the review-eight "no P2", "generated checks" twice, "dropped first", "removing the check is killed", review seven's "the one P2"). | Corrected in this commit. | this file | | |
| + |
| +Open after this round: an operator's door to clear a quantity-history gap whose cause the operator has established (none exists; the account stays held); the site's `bands.p3` sentence; two unreviewed system strings on the billing page; everything that needs Stripe test mode. |
| + |
| +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-review9.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review9.md| new file mode 100644 |
| index 000000000..551e66c93 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review9.md |
| @@ -0,0 +1,103 @@ |
| +# Ninth review of `feat/carloku-multi-currency-20261001` at `f8a23ef3` (base `12e70d95`) |
| + |
| +**Verdict: accept with fixes.** All five eighth-round findings are closed in the code. But one of the fixes, the startup sweep that "heals" quantity-history gaps, introduces a new P2: it also clears gaps the console sets on purpose for two other reasons. It should be removed or narrowed before merge. |
| + |
| +How I worked: |
| +- I read only with `git show f8a23ef3:`, modified nothing and ran no tests. |
| +- The requested delta `f8a23ef3..f8a23ef3` is empty, so I read `3e941f00..f8a23ef3` as the eighth-round fixes. |
| +- That delta touches `billingservice.go` (the binder), `billingquantityaudit.go` (the heal sweep), `billingstore.go` (the review reference), `billingoperator.go`, one comment in `billinghandlers.go`, the tests, the design and the dispositions. |
| +- `internal/plans`, `cmd`, `billingpage.go`, the trigger code and the site at `9427c22` are unchanged. The site's `pricebook.json` is byte-identical to `internal/plans/public-pricebook.json` at `f8a23ef3`; its `pricebook.source.txt` still names `5047494b`. |
| +- **Not re-traced:** code that did not change this round, which earlier rounds traced in full. For A, C, E–H, J, K and N–P I checked only how the new code interacts with it. |
| + |
| +## Eighth-review findings |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Astra 1 (P2)**: a terms or plan or band mismatch was refused before ownership, with no review | **Closed.** `bindBillingCheckoutFor` now runs `ownSession` first (`billingservice.go:964`). It then judges plan, band, terms, price terms and currency together and files the one review on any mismatch (`:967-980`). The `checkout_intent` comparison that was dropped from the early check is still in `ownSession` (`:260`). | **No.** In the new case (`billingcurrency_test.go:2366-2400`), the pass reaches the binder through the `complete` branch (`billingpass.go:198-200`). Under the old order, both the pass leg and the fresh-webhook leg return `errBillingIdentity` with 0 reviews. | |
| +| **Astra 2 / Opus P3-2**: the reference was not visible to the operator | **Closed** for the store reader (`billingoperator.go:61,69`) and the API (`:178`, `:198`). There is no CLI consumer of `/api/billing/reviews` in this tree. | **No.** `TestOperatorSeesTheRefusedSessionWhileTheReviewIsOpen` reads `openBillingReviews`, and the API leg checks `"reference":"cs_test_paid"`. | |
| +| **Astra 3 (P3)**: an already-open review never learned its reference | **Closed.** The fill now runs whether or not a review was already open, only where `reference=''`, and adds no occurrence (`billingstore.go:980-992`). | **No.** The test blanks the reference and lets the pass meet the session again. Under the old early return, the reference stays `''`. | |
| +| **Opus P3-1**: a rollback held the account for good | **Closed for the documented path, with a new defect.** The roll note was added (dispositions line 70). The heal sweep added beside it is P2-1 below. | **No.** `TestStartupHealsAGapFromAnOutcomeAdmittedSince` fails without the heal. Its only negative case is a bad outcome, so it cannot see P2-1. | |
| +| **Claims (both)** | **Partly corrected.** Fixed: the membership-lock wording, the fixture comment, and the webhook status assertions. Four claims are marked corrected but are still in the text (below). | n/a | |
| + |
| +**Where the reviewers disagreed.** Astra rated finding 1 P2; Opus did not report it. From the code, **P3 is the better reading:** |
| +- Reaching it needs the authenticated provider to return an owned, completed session whose `terms_version`, `plan` or `band` metadata differs from what the console set at creation (`checkoutParams`). No buyer or tenant can cause that. |
| +- The currency mismatch the review exists for has a realistic provider-side cause (conversion, Adaptive Pricing). A metadata rewrite has none. |
| +- The consequence was real but bounded to that one account: money taken, no review, and the standing intent blocking a new purchase. |
| + |
| +On the round-seven dispute the eighth reports revisited (the binder's second read), I agree with Opus's P3 on reachability. Astra's point about the cross-account consequence is fair, and the fix is in either way. |
| + |
| +## Findings |
| + |
| +### P2-1. The new heal sweep clears gaps that were set on purpose, by the pre-history summary and the clock-rollback hold |
| +- **Where:** `internal/console/billingquantityaudit.go:53-57` (`UPDATE billing_quantity_changes SET history_gap=0 WHERE history_gap<>0 AND state='history_unproven' AND NOT EXISTS(<a bad processed event>)`). It runs on every start, through `migrateBilling` → `migrateBillingMembership` → `migrateQuantityHistoryGuards`. |
| +- **What it misses:** `history_gap` has three writers, and all three produce `state='history_unproven'` (`billingmembership.go:478-479`): |
| + 1. The startup sweep's mark (`billingquantityaudit.go:42`), the only one the heal is meant for. |
| + 2. The **pre-history default**. The column is added with `DEFAULT 1` so that a summary written before the event history existed is held "for good" (`billingmembership.go:144-147`). The next summary write keeps the gap and sets `history_unproven`, as `billingmembership_test.go:714` asserts. |
| + 3. The **clock-rollback hold**: `clockReversed` at `billingmembership.go:470`, asserted at `billingquantityaudit_test.go:180-189`. |
| +- **Why the heal cannot tell them apart:** causes 2 and 3 are not processed events with a bad outcome. Their accounts' events are all ordinary (`charged`, `included`, …), so the `NOT EXISTS` is true and the heal clears them. |
| +- **Scenario (clock):** |
| + 1. A Business account's host clock steps back a minute. A member added in that minute writes an event whose `occurred_at_ns` is below an earlier generation's. |
| + 2. `writeQuantitySummaryTx` sets `history_gap=1, state='history_unproven'`, and the account is held (`quantityObligationTx`). That is intended. |
| + 3. Both events are processed normally. |
| + 4. The console restarts (any roll or reboot). The heal clears the gap. |
| + 5. Months freeze and invoice, and `reserveBillingRetirement` allows deletion, over a history the code had declared unprovable. |
| +- **Scenario (pre-history):** |
| + 1. A summary from before the event table took the default gap. A membership change then made it `history_unproven`. |
| + 2. The next start clears it. |
| + 3. An add-then-remove before history began, which may have owed a proration, is now never held. |
| +- **Why the tests miss it:** neither existing test restarts the store. The new test's only negative case is a corrupted outcome. |
| +- **Smallest acceptable fix:** remove the automatic heal and rely on the roll note, which already forbids the rollback. If the heal is wanted, it needs its own evidence of cause, for example a `gap_cause` column written by this build, and a base-binary mark cannot carry one. So: an audited operator door that clears one account's gap, writing a row like `billing_review_resolutions`. |
| +- **Tests to add:** re-run `migrateBilling` after the gap is set in the pre-history test and in `TestQuantityAuditClockRollbackPreservesEventAndHolds`, and assert `history_gap` is still 1. Both fail at `f8a23ef3`. |
| + |
| +### P3-1. The resume and open-session paths do not judge plan or band, which the binder now does |
| +- **Where:** |
| + - `intentSession` (`billingservice.go:243-251`) checks ownership and `priceTerms()`, which are terms version, book and currency. It does not check `plan` or `band`. |
| + - The binder refuses and reviews on `plan` or `band` (`:967`). |
| + - The pass's open branch (`billingpass.go:214`) and the checkout's resume both rely on `intentSession`. |
| +- **Scenario:** |
| + 1. The provider returns the intent's own open session with `metadata.plan` or `band` altered (provider-only, as for Astra 1). |
| + 2. Resume treats it as valid and hands the buyer its URL. |
| + 3. The buyer pays. |
| + 4. Only then does the binder file the review. The paid-then-reviewed outcome is avoidable while the session is still open: it could be expired instead. |
| +- **Smallest fix:** add the plan, band and `terms_version` comparisons to `intentSession`, so that an open mismatch is refused and expired by `refuseSession`. |
| + |
| +## Sections (only what changed or interacts) |
| + |
| +- **A.** The GBP purchase is unchanged end to end. The binder reorder touches no amount, and the heal touches no money directly; P2-1 is a hold, not a conversion. I found no path that converts. |
| +- **D / L / R / W / Y / AA.** `ownSession` now precedes every purchase-identity judgment in the binder. |
| + - With an expected intent, ownership and `account_sqid` are checked against the intent's own account before any account is derived (`:931-939`). |
| + - The webhook (nil) path derives the account, requires `client_reference_id`, then runs `ownSession` against that account's intent. |
| + - The early return for an already-recorded subscription (`:950`) still precedes `ownSession`. It is benign: it records nothing, and reconcile checks independently. |
| + - A review can be filed only through `fileCheckoutRefusedReview`, which requires the exact intent to stand and is once while open. That holds on every path: pass, webhook, success URL and direct bind. |
| +- **S / V / Z / BB.** |
| + - The reference fill's `UPDATE` does not filter `resolved_at=0`. It is unreachable in practice: the fill requires the intent to stand, and resolution deletes that intent in the same transaction. |
| + - The reference is first-writer-wins. For an unbound intent, `ownSession` does not pin the session id, so if the provider listed two owned paid sessions, the first one met is recorded. That is acceptable, since one key means one session. |
| + - Resolution reads `reference` without filtering by occurrence (`billingstore.go:928`). That is correct, because the key is unique. |
| +- **X.** There is still one list (`quantityOutcomesSQL`). The heal reuses it; its fault is P2-1, not the list. |
| +- **H / N / T.** |
| + - Older binary against a migrated database: the roll note now covers the resolved-refusal case. "Before the eighth-round commit" is conservative, since `3e941f00` already knows the outcome. |
| + - Trigger migration: unchanged since round 7. No state leaves neither trigger, and both present fails closed. |
| +- **B / C / E / F / G / J / K / M / O / P / Q / U.** No code change this round, and the eighth reports' traces stand. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **Dispositions, review eight:** "neither found a P1 or a P2 in the code's money paths". Astra's finding 1 was a P2, in the binder. |
| +2. **Dispositions, review eight:** lists as corrected "generated checks" in two rows and "removing the check was killed". All are still in the text: |
| + - line 79: "its test now reads the generated checks"; |
| + - line 120: "the plans test reads the generated checks"; |
| + - line 143: "(a mutant removing the check is killed)". |
| +3. **Line 116:** "the old one is dropped first at migration". It is dropped after the new one is confirmed (`retireEarlierBarrierTrigger`). Astra flagged this in round eight, and it is unchanged. |
| +4. **Line 176:** "The two agree on every finding this round, including the one P2". Astra rated the binder's second read P2 as well. |
| +5. **Dispositions, review eight and roll note:** "this binary's sweep clears such a mark again … so a roll back and forward heals" and "a gap it (or an earlier binary) marked". It also clears gaps it did not mark (P2-1). |
| +6. **`billingmembership.go:144`:** "keeps history_gap=1 for good" is now false across a restart (P2-1). |
| + |
| +## Before enabling: Stripe test mode |
| + |
| +The eighth review's fourteen items stand: catalog and pager; the currency × country matrix including IN with GBP; the refusal shape; the subscription's currency and metadata; the customer currency lock; GBP and USD overage invoices; proration and renewal per currency; lost-response replay and listing; expiring a completed session; a replayed completion after a resolution; promotion codes; a listed refusal resolved, with the next month freezing, the id in `billing_retired_intents`, and now also in `GET /api/billing/reviews` while open; a concurrent webhook and pass giving one review; and the rollback rehearsal. |
| + |
| +Add: |
| + |
| +15. **Owned completed session with altered metadata.** In test mode, confirm that a session's `metadata.terms_version`, `plan` and `band` cannot change after creation; checking `plan` and `band` matters most. If they cannot, Astra 1 and P3-1 are provider-impossible and remain belt and braces. |
| +16. **Not Stripe, in the deployment's environment.** Restart the console against a copy of a database holding a clock-rollback hold and a pre-history summary, and confirm the gap survives. It will not at `f8a23ef3` (P2-1). |
| + |
| +**Verdict: accept with fixes.** Remove or narrow the heal sweep (P2-1), add the two restart assertions, and correct the six stale claims; P3-1 can follow. |
diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go| index b0837dcd5..ecb969fec 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -415,6 +415,25 @@ func TestCheckoutNeverOffersASessionInAnotherCurrency(t *testing.T) { |
| } |
| noIntent(t, fx, acct) |
| }) |
| + t.Run("an open session whose plan, band or terms changed is refused and expired, not offered", func(t *testing.T) { |
| + for field, value := range map[string]string{"plan": plans.PlanBusiness, "band": plans.BandD, "terms_version": "2031-01-01-v9"} { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := click(fx, f, acct, 0); err != nil || dest == "" { |
| + t.Fatalf("%s: first click: dest %q err %v", field, dest, err) |
| + } |
| + f.mu.Lock() |
| + f.meta[field] = value |
| + f.mu.Unlock() |
| + if dest, err := click(fx, f, acct, time.Minute); dest != "" || !errors.Is(err, errBillingIdentity) { |
| + t.Fatalf("%s changed on the open session: dest %q err %v, want no destination", field, dest, err) |
| + } |
| + if f.sessionStatus != "expired" || f.expireCalls != 1 { |
| + t.Fatalf("%s changed: the session a buyer may still hold the URL of is %s (%d expiries)", field, f.sessionStatus, f.expireCalls) |
| + } |
| + noIntent(t, fx, acct) |
| + } |
| + }) |
| t.Run("a created session that is not this intent's is not kept", func(t *testing.T) { |
| fx, f, acct := newCurrencyFixture(t) |
| f.customerCountry, f.createdIntent = "GB", "0123456789abcdef0123456789abcdef0123456789abcdef" |
| @@ -3089,18 +3108,18 @@ func TestOperatorSeesTheRefusedSessionWhileTheReviewIsOpen(t *testing.T) { |
| } |
| } |
| -// A quantity-history gap the startup sweep marked under an earlier list of |
| -// outcomes is cleared again on the next start when every processed event |
| -// carries an outcome the list has now; a gap with a real bad outcome |
| -// behind it stays. |
| -func TestStartupHealsAGapFromAnOutcomeAdmittedSince(t *testing.T) { |
| +// A quantity-history gap an earlier binary's sweep marked, for an outcome |
| +// this binary admits, stays held across this binary's starts: a gap's |
| +// cause is not recorded, and a sweep that cleared such marks would clear |
| +// the pre-history and clock-rollback holds too. The roll note forbids the |
| +// rollback that makes one; clearing one is an operator's door that does |
| +// not exist yet. |
| +func TestAGapFromAnObsoleteOutcomeListStaysHeld(t *testing.T) { |
| fx, _, acct := newCurrencyFixture(t) |
| if _, err := fx.st.db.Exec(`INSERT INTO billing_quantity_changes(account_id,generation,plan,terms,live_mode,members,desired_quantity,billed_quantity,billing_revision,intent_id,customer,subscription,period_start,period_end,history_gap,state,updated_at) |
| VALUES(?,1,'community','t',1,11,11,10,1,'','','',0,0,0,'in_sync',1)`, acct.ID); err != nil { |
| t.Fatal(err) |
| } |
| - // An event, appended unprocessed as the console appends them, then |
| - // processed with the outcome the resolution writes. |
| if _, err := fx.st.db.Exec(`INSERT INTO billing_quantity_events(account_id,generation,occurred_at_ns,change,effect,address,members_before,members_after,desired_quantity,billed_quantity,plan,terms,live_mode,customer,subscription,intent_id,billing_revision,period_start,period_end) |
| VALUES(?,1,1000000000,'addition','prorate','kim@keymail.dev',10,11,11,10,'community','t',1,'','','i1',1,0,0)`, acct.ID); err != nil { |
| t.Fatal(err) |
| @@ -3117,37 +3136,16 @@ func TestStartupHealsAGapFromAnOutcomeAdmittedSince(t *testing.T) { |
| t.Fatal(err) |
| } |
| var gap int |
| - if err := fx.st.db.QueryRow(`SELECT history_gap FROM billing_quantity_changes WHERE account_id=?`, acct.ID).Scan(&gap); err != nil || gap != 0 { |
| - t.Fatalf("the gap after this binary's start: %d %v; want it cleared", gap, err) |
| + if err := fx.st.db.QueryRow(`SELECT history_gap FROM billing_quantity_changes WHERE account_id=?`, acct.ID).Scan(&gap); err != nil || gap != 1 { |
| + t.Fatalf("the gap after this binary's start: %d %v; want it kept (no sweep can tell its cause)", gap, err) |
| } |
| - // A gap with a bad outcome behind it stays. (The events are append-only |
| - // and guarded; the bad row is written with the guards lifted, as a |
| - // corruption would be, and the migration puts the guards back.) |
| - rows, err := fx.st.db.Query(`SELECT name FROM sqlite_master WHERE type='trigger' AND (tbl_name='billing_quantity_events' OR sql LIKE '%billing_quantity_events%')`) |
| + tx, err := fx.st.db.Begin() |
| if err != nil { |
| t.Fatal(err) |
| } |
| - var triggers []string |
| - for rows.Next() { |
| - var name string |
| - if err := rows.Scan(&name); err != nil { |
| - t.Fatal(err) |
| - } |
| - triggers = append(triggers, name) |
| - } |
| - rows.Close() |
| - for _, name := range triggers { |
| - if _, err := fx.st.db.Exec(`DROP TRIGGER IF EXISTS ` + name); err != nil { |
| - t.Fatal(err) |
| - } |
| - } |
| - if _, err := fx.st.db.Exec(`UPDATE billing_quantity_events SET processing_outcome='nonsense' WHERE account_id=?`, acct.ID); err != nil { |
| - t.Fatal(err) |
| - } |
| - if err := fx.st.migrateBilling(); err != nil { |
| - t.Fatal(err) |
| - } |
| - if err := fx.st.db.QueryRow(`SELECT history_gap FROM billing_quantity_changes WHERE account_id=?`, acct.ID).Scan(&gap); err != nil || gap != 1 { |
| - t.Fatalf("the gap with a bad outcome behind it: %d %v; want it kept", gap, err) |
| + held, err := quantityObligationTx(tx, acct.ID) |
| + tx.Rollback() |
| + if err != nil || !held { |
| + t.Fatalf("the account with a kept gap is not held (%v %v)", held, err) |
| } |
| } |
diff --git a/internal/console/billingmembership_test.go b/internal/console/billingmembership_test.go| index f6f9ec4fb..d23be7081 100644 |
| --- a/internal/console/billingmembership_test.go |
| +++ b/internal/console/billingmembership_test.go |
| @@ -714,6 +714,14 @@ func TestQuantityHistoryMigrationMarksOldSummaryUnproven(t *testing.T) { |
| if !q.HistoryGap || q.State != quantityChangeHistoryUnproven || q.Members != 9 { |
| t.Fatalf("pre-history summary reads as proven: %+v", q) |
| } |
| + // And it stays so across a restart: no sweep clears a gap whose cause |
| + // it cannot tell. |
| + if err := s.migrateBilling(); err != nil { |
| + t.Fatal(err) |
| + } |
| + if q, _ = f.quantityChange(); !q.HistoryGap || q.State != quantityChangeHistoryUnproven { |
| + t.Fatalf("a restart cleared the pre-history gap: %+v", q) |
| + } |
| // Accounts first summarised after the migration have a complete history. |
| g := newTeamMembershipFixture(t) |
| if err := g.s.AddMember(g.id, "a@example.test", RoleMember, g.now); err != nil { |
diff --git a/internal/console/billingquantityaudit.go b/internal/console/billingquantityaudit.go| index 71d6a4c80..bc21634ec 100644 |
| --- a/internal/console/billingquantityaudit.go |
| +++ b/internal/console/billingquantityaudit.go |
| @@ -43,18 +43,13 @@ func (s *Store) migrateQuantityHistoryGuards() error { |
| 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 |
| } |
| - // And the inverse, for a gap this sweep itself marked (state |
| - // history_unproven) under an earlier list of outcomes: an account none |
| - // of whose processed events carries an outcome outside the list now is |
| - // not in doubt, and a binary rolled back across a resolved refusal and |
| - // forward again must not hold it for good. The state label is left for |
| - // the next summary write to recompute; the gap, which is what holds the |
| - // account (quantityObligationTx), is cleared. |
| - if _, err := s.db.Exec(`UPDATE billing_quantity_changes SET history_gap=0 |
| - WHERE history_gap<>0 AND state='history_unproven' |
| - AND NOT 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 |
| - } |
| + // No sweep clears a gap. A gap's cause is not recorded: the pre-history |
| + // default, a clock that stepped back and this sweep's own mark all read |
| + // the same (history_unproven), so a sweep that cleared marks it took to |
| + // be its own would clear the others too (review nine of the currency |
| + // branch). A mark an earlier binary made for an outcome this one |
| + // admits stays until an operator clears it; the roll note forbids the |
| + // rollback that makes one. |
| return nil |
| } |
diff --git a/internal/console/billingquantityaudit_test.go b/internal/console/billingquantityaudit_test.go| index 73275c0aa..fb07bb9ab 100644 |
| --- a/internal/console/billingquantityaudit_test.go |
| +++ b/internal/console/billingquantityaudit_test.go |
| @@ -187,6 +187,13 @@ func TestQuantityAuditClockRollbackPreservesEventAndHolds(t *testing.T) { |
| if !q.HistoryGap || q.State != quantityChangeHistoryUnproven { |
| t.Fatalf("later reconciliation erased clock hold: %+v", q) |
| } |
| + // And a restart does not erase it either. |
| + if err := f.s.migrateBilling(); err != nil { |
| + t.Fatal(err) |
| + } |
| + if q, _ = f.quantityChange(); !q.HistoryGap || q.State != quantityChangeHistoryUnproven { |
| + t.Fatalf("a restart erased the clock hold: %+v", q) |
| + } |
| } |
| func TestQuantityAuditPendingAdditionHoldsRetirementAndFreeze(t *testing.T) { |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index f56856bb0..9d7e58c30 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -244,6 +244,9 @@ func (b *billingRuntime) intentSession(session *billingstripe.Session, intent bi |
| if !b.ownSession(session, intent, acct) { |
| return false |
| } |
| + if session.Metadata["plan"] != intent.Plan || session.Metadata["band"] != intent.Band || session.Metadata["terms_version"] != intent.Terms { |
| + return false |
| + } |
| terms, err := priceTermsFromMetadata(session.Metadata) |
| if err != nil || terms != intent.priceTerms() { |
| return false |