amadan
Review seventeen answered: the review filed before the expiry on both paths, the keep door's replay, nil billing and purchase check, the rejected listing's account, order and bound
Astra's seventeenth review found a P2 in the refusal's own order: both sites that expire a bound session (reconcile's refusal of a first purchase and ownBoundSession under every reader) expired first and filed the review second, so a pass reading the expired session in between, or a crash between the two, retired the intent with no record. The review is now filed first at both sites; with it open no ordinary retirement can take the intent, and a failed expiry is tried again at the next read. A test runs the store's expiry-with-proof retirement inside the fake's expiry on each path and wants the conflict; with either site's order put back it fails. Opus's four P3s and Astra's: an exact keep replay after the corrected purchase recorded answered 409 (the handler now reads the review first and lets a resolved occurrence through to the store's replay comparison); keep dereferenced a nil billing service when billing is off (409 before the gate); keep was accepted for a completed session that is not the intent's purchase (the gate requires intentSession); the rejected-events listing never carried an account (reconcile's retired-settle returns a typed identity error carrying it), ordered ties oldest first (rowid DESC), and its test checked neither order nor account. The first form of the account fix looked the account up under the open rows and deadlocked the store's one connection; the rows are read to the end first. Two tests added: an older resolutions table with rows migrates with retire as each row's outcome and refuses a keep replay; the listing is bounded at 200, newest first, the account on each row. Comments at the refusal, the retirement and the operator's resolution, the store's filing comment, the operator file's header and the design's sentence on unknown terms corrected. Both reports copied in; dispositions "Review seventeen". Site fixes (render before the draft check; a fractional-cent floor refused) are on the site branch at 4026231.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
10 files changed,
+583
−47
docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md+1 −1docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review17.md+169 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+20 −1docs/superpowers/specs/OPUS-REVIEW-multi-currency-review17.md+109 −0internal/console/billingcurrency_test.go+175 −0internal/console/billinghandlers.go+11 −5internal/console/billingoperator.go+47 −19internal/console/billingservice.go+38 −18internal/console/billingservice_test.go+9 −0internal/console/billingstore.go+4 −3
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 c26671f76..b8386c6f6 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 |
| @@ -140,7 +140,7 @@ func PublicPriceBook(book string) ([]byte, error) // the export below, byte-stab |
| `Quote`, `CalculateOverage`, `InvoiceOverageLines` and the `MicroEUR` types stay as they are, for legacy rows and for snapshots already written. The new functions give the same numbers for legacy terms. |
| -`Assignment` gains `Currency` and `PriceBook` (both `omitempty`, so an existing snapshot marshals byte for byte as before). `AllowanceForMonth` copies them to `MonthAllowance` and refuses, with `ErrPriceTerms`, a month whose paid spans disagree on either or name terms this build does not have; the invoice engine files a review for such a month rather than retrying it quietly. |
| +`Assignment` gains `Currency` and `PriceBook` (both `omitempty`, so an existing snapshot marshals byte for byte as before). `AllowanceForMonth` copies them to `MonthAllowance` and refuses, with `ErrPriceTerms`, a month whose paid spans disagree on either, and with `ErrUnknownTermsVersion` one that names terms this build does not have; the invoice engine files a review for the first and retries the second (a newer build may know the terms). |
| ## Console data contract |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review17.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review17.md| new file mode 100644 |
| index 000000000..e9e9a1afa |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review17.md |
| @@ -0,0 +1,169 @@ |
| +The sixteenth review’s behavioral findings are closed for their reported scenarios. The new `keep` check introduces a replay regression, and I found a separate race before the refusal review is committed. |
| + |
| +All platform references below are at **`db130cfc`**; pricing-site references are at **`c12318d`**. I modified nothing and ran no tests, network commands or AWS commands. Test counterfactuals are from source inspection. |
| + |
| +| Sixteenth-review finding or claim | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1:** `keep` accepts an expired session whose purchase cannot record | **Yes, for that scenario.** `billingKeepRefused` requires the standing intent’s owned, bound session to be complete and carry a subscription (`internal/console/billingoperator.go:225`). | **No.** The assertion at `billingcurrency_test.go:3989` would receive 200 instead of 409; the review would also be closed. | |
| +| **Opus P3-2:** outcome missing from history, replay identity and audit | **Yes.** Default migration, persistence, comparison and audit are present (`billingstore.go:227`, `:948`, `:967`; `billingoperator.go:334`). | **No.** Removing the comparison fails the opposite-outcome replay at `billingcurrency_test.go:3960`; removing persistence or audit fails `:3964` or `:4003`. | |
| +| **Opus claim 1:** site test falsely claims literal provenance | **Yes.** Its title and comment now explicitly promise output equality (`site/test/pricing-pricebook.test.mjs:190`). | **Yes** for an identical literal. The correction accurately acknowledges that limitation. | |
| +| **Opus claim 2:** later retirement lookup is redundant | **Yes.** Removed; the early check remains (`billingservice.go:801`, `:890`). | Removing the redundant lookup changes no result. Removing the early check fails the unknown-terms retired-event cases. | |
| +| **Opus claim 3:** design describes `keep` without its necessary condition | **Yes, for the reported expired-session defect.** Design line 152 now states the completed-session condition. | The expired-session regression distinguishes the behavioral fix; prose itself has no behavioral test. | |
| +| **Astra 1:** opposite outcomes replay successfully and the decision is unrecorded | **Yes**, with the same evidence as Opus P3-2. Exact same-outcome replay now has Finding 2 below. | **No** for the tested `keep` → `retire` replay. The reverse direction and exact `keep` replay after recording are not tested. | |
| +| **Astra claims:** resolution history, site provenance, review-four lookup coverage, unknown-terms wording | **Corrected.** Dispositions lines 108 and 292 now qualify the claims accurately. | Historical mutation runs cannot be established from source. The delete-only mutant remains redundant behind the exact-intent lookup. | |
| +| **Astra claim:** unconditional “resolution retires” comments | **Not fully closed.** Three cited service comments remain unchanged; see claims below. | No behavioral test establishes comment accuracy. | |
| + |
| +The supplied sixteenth reports do **not** contain a P2-versus-P3 disagreement: both classify the outcome defect P3. Their substantive disagreement concerns the expired-session `keep`: Opus reported it; Astra described the subsequent ordinary retirement without treating the resulting stranded subscription as a finding. **Opus’s reading was right:** an expired session cannot satisfy reconciliation, ordinary retirement writes no retirement record, and a subsequently corrected subscription cannot record or settle through that record. |
| + |
| +On the earlier bound-session expiry disagreement, expiring the **previously validated, stored bound ID** is justified. Expiring an unowned ID learned only from a create response or listing is not. |
| + |
| +## Findings |
| + |
| +1. **P2 — A pass can retire the intent between refusal expiry and review filing.** |
| + |
| + **Where:** `internal/console/billingservice.go:829`, `:834`; `billingpass.go:209`; `billingstore.go:457`, `:1032`. |
| + |
| + **Scenario:** A hand-made subscription names the standing intent but has the wrong band or price. Reconciliation expires that intent’s open bound session. Before it files the review, the background pass reads the now-expired, correctly owned session and retires the intent. The deletion succeeds because no review exists yet. The subsequent `fileCheckoutRefusedReview` sees no standing intent and silently does nothing. |
| + |
| + Reconciliation’s account lock does not protect this sequence: the pass’s expiry branch takes no such lock. A crash after provider expiry and before review filing creates the same gap. |
| + |
| + The original rejection can be listed, but there is **no actionable refusal review and no retirement record**. A replacement checkout can open beside the existing provider subscription. If the original subscription is corrected, its events reach “no intent stands” and retry with 503. |
| + |
| + **Smallest acceptable fix:** establish a durable refusal reservation before expiring the session. Ordinary retirement must respect it; expiry failures must retain a retryable reservation, and resolution must not release an unresolved expiry. Finish the review transition against the exact intent. |
| + |
| + The existing test at `billingcurrency_test.go:3749` runs the pass **after** review filing (`:3782`). It passes without protection for this interleaving. Add a controlled pause after provider expiry and run the competing pass there. |
| + |
| +2. **P3 — An exact successful `keep` replay returns 409 after the purchase records.** |
| + |
| + **Where:** `internal/console/billingoperator.go:310`, `:316`; `billingstore.go:937`. |
| + |
| + **Scenario:** The operator resolves with `keep`; the corrected purchase records and consumes its intent. The operator retries the identical resolution—for example, because the first HTTP response was lost. `billingKeepRefused` now answers “no such intent stands”, so the handler returns 409 **before reaching the persisted exact-replay comparison**. A provider read failure can similarly turn an already-committed replay into 503. |
| + |
| + **Smallest acceptable fix:** recognize an exact committed resolution before applying the provider precondition for a new `keep`. Preserve occurrence, actor, note and outcome checks, and return the committed decision. |
| + |
| + `TestACorrectedPurchaseWaitsForTheOperatorUnderItsReview` reaches precisely the consumed-intent state, but only retries with the **opposite** outcome (`billingcurrency_test.go:3960`). It passes with this regression. |
| + |
| +3. **P3 — `keep` panics when billing has been switched off.** |
| + |
| + **Where:** `internal/console/billingoperator.go:236`, `:310`; `internal/console/server.go:1155`. |
| + |
| + **Scenario:** An account has a bound intent under review. Billing is disabled and the console restarts. Operator review APIs remain registered and usable, but `keep` dereferences `s.billing.client` when `s.billing` is nil. The request panics instead of returning a controlled refusal; the review remains unresolved. |
| + |
| + **Smallest acceptable fix:** for a new `keep`, return 503 when the billing runtime is unavailable, leaving the review open. Keep read-only review access and default `retire` available. Add a disabled-runtime case; the current regression fixture always supplies billing. |
| + |
| +4. **P3 — The site build can validate stale generated HTML instead of the selected export.** |
| + |
| + **Where:** site `hack/check-pricebook.mjs:6`, `package.json:6`, `:8`; `test/pricing-pricebook.test.mjs:16`. |
| + |
| + **Scenario:** An approved generated page remains in `src/pricing/index.html` when a newer draft export is copied into the repository. The checker reads only the old HTML’s marker and reports approval. `npm run build` then publishes that old page without invoking the price-book renderer. |
| + |
| + `npm run check` also builds Eleventy output before its pricing test regenerates the source page with `PRICEBOOK_ALLOW_DRAFT=1`. It can therefore inspect regenerated source while leaving different, stale HTML in `_site`. |
| + |
| + **Smallest acceptable fix:** generate pricing HTML from the selected export, enforce its status, and only then run Eleventy. Verify the same artifact that will be published; tests should not silently regenerate source after that artifact was built. |
| + |
| + The committed draft marker works correctly. The missing check is agreement between the export, generated source and deployable output. |
| + |
| +5. **P3 — A valid fractional-cent invoice floor produces malformed site text, and the test repeats the bug.** |
| + |
| + **Where:** site `copy-review/pricing/build.mjs:41`, `:322`; `test/pricing-pricebook.test.mjs:29`, `:144`, `:217`. Platform acceptance: `internal/plans/pricebook.go:330`. |
| + |
| + **Scenario:** A subsequent valid book sets the USD floor to `1,205,000` micro-units. Platform validation accepts it. The site divides by 10,000 and passes `120.5` to a formatter expecting integral minor units, producing **`$1.20.5`**. |
| + |
| + The floor assertions and allowed-amount set use the same formatter, so they agree with the malformed result. This does not affect the committed whole-cent floors. |
| + |
| + **Smallest acceptable fix:** either reject non-integral minor-unit floors during rendering, or format the exported micro amount exactly. Test the displayed numeric value independently of the rendering helper. |
| + |
| +## Other requested paths |
| + |
| +**A — GBP end to end.** `GET ?country=IN¤cy=gbp` produces India’s band D and an explicit GBP choice. Personal is **£7.70 annually**, with **29.75 awake hours, 0.7 GB storage and 0.7 GB transfer** (`billingpage.go:265`, `:395`; `billingcurrency_test.go:2637`). |
| + |
| +The posted country, effective currency and signed quote must agree (`billinghandlers.go:71`, `:102`); India also requires postal information. Checkout prices and verifies the GBP provider price, then stores the book and GBP on the intent (`billingservice.go:482`, `:514`, `:576`). `CreateCheckout` receives those pinned terms with fixed currency and Adaptive Pricing disabled through `checkoutParams` (`:221`). Binding and first reconciliation compare ownership and purchase identity (`:1130`, `:900`, `:942`). |
| + |
| +The recording transaction preserves those terms in account state and book assignments (`billingstore.go:614`, `:740`). Invoice history reads both assignment tables (`billinginvoicestore.go:529`); `AllowanceForMonth` retains one paid-span currency/book (`internal/plans/month.go:173`); calculation version 2 prices from those terms (`billingcalculation.go:183`). Draft invoices, items, recovered objects and final lines are checked against that calculation’s currency (`billinginvoice.go:859`, `:901`, `:964`, `:1024`, `:1116`). Quantity reads use the stored price and both subscription/item currencies (`billingquantity.go:1014`). |
| + |
| +I found no conversion or cross-currency recording path for that purchase. |
| + |
| +**B/C/M/O/Q/U/Z — Form, signature, cookies and held currency.** |
| + |
| +- A present currency field must have exactly one supported lowercase value. Empty, repeated, padded or unsupported values receive 400 on both doors, after the preceding authentication/form gates (`billinghandlers.go:176`). |
| +- Missing currency uses the country default on a book console and EUR on a legacy console, subject to signed-quote comparison. Forged `quoted_*` fields or a changed choice flag cannot manufacture a signature. |
| +- The HMAC key is derived from the billing secret. Its message includes session token, account, book, country, band, currency, choice flag and member count (`billingpage.go:446`). Disclosed form values do not disclose the key. Another session/account/book, changed token or changed signed tuple invalidates it. |
| +- An old tab may reuse its own unchanged quote. Another tab’s cookies do not change what that form posts. Membership changes cause a conflict under the billing lock, followed by the membership-locked recount when creating the intent (`billingservice.go:467`; `billingstore.go:353`). |
| +- Plan is unsigned because all offered plan prices were displayed. Quantity follows membership and plan limits. Postal information affects tax determined at checkout; it does not alter the quoted base price, band or currency. |
| +- The quote endpoint checks session, token and membership, then redirects to the account’s fixed billing path with encoded parameters. It creates no purchase (`billinghandlers.go:137`). |
| +- Present-empty URL parameters clear cookies; absent parameters fall back; invalid parameters leave cookies unchanged (`billingcurrency.go:43`). Consequently, an invalid visit can leave an earlier choice available on a later parameter-absent visit. Explicit clearing cannot. |
| +- A previous purchase offers only its held currency, including legacy EUR. URL and cookie cannot override it; checkout independently refuses another currency (`billingpage.go:301`; `billingservice.go:563`). |
| + |
| +**D/L/R/W/Y/AA/DD/EE/FF/HH–NN — Ownership and refusals.** |
| + |
| +`ownSession` checks mode, both account identifiers, intent ID, known customer and bound session ID (`billingservice.go:272`). Both create sites, recovery listing and resume paths use it before accepting provider proof. The expected-intent binder checks against that intent’s account before deriving an account from the response (`:1081`). |
| + |
| +There are deliberate exceptions to requiring ownership before expiry: `ownBoundSession` and reconciliation’s `refuse()` act on the **console’s stored bound ID**, after confirming the read returned that ID (`:297`, `:818`). Their expiry answers do not authorize retirement. Unowned create/listing responses confer no equivalent authority; those sessions remain open at the provider, which is the correct refusal to act on an unproved ID. |
| + |
| +Read failures, another-ID answers and expiry failures retry rather than authorize resolution. Completion during expiry can make expiry fail; the next read then judges the completed session. Concurrent attributable mismatches use one intent-keyed open review, including its reference (`billingstore.go:1025`). Finding 1 concerns the interval **before** that review exists. |
| + |
| +Reconciliation uses `intentSession` after ownership, matching the binder’s purchase judgment. Correct recorded subscriptions and renewals need no standing intent. A membership change does not legitimately change the initial intent’s provider quantity before recording. Wrong-intent subscriptions are settled when they reach identity refusal; unknown terms, incomplete status and missing bindings can return earlier as retries. |
| + |
| +**S/V/GG/OO/RR–WW/XX — Lifecycle.** The implemented invariant is: |
| + |
| +> Once an intent’s refusal review is committed, ordinary retirement and first-purchase recording cannot consume that intent. `retire` archives and deletes exactly it; `keep` removes the hold while leaving all purchase validations in force. A recorded retirement prevents subsequent activation. |
| + |
| +| Path | Result | |
| +|---|---| |
| +| Checkout, recovery and resume | Cannot replace a standing intent; ownership/purchase checks precede acceptance. | |
| +| Background pass and account-deletion recovery | Use guarded binding/retirement. Finding 1 is the unguarded interval before review filing. | |
| +| Stillborn cleanup | Deletes only an unbound intent with `customer_ready=0`, before session creation is possible. | |
| +| Binder, webhook and direct binding | Conditional binding cannot resurrect a deleted intent. The already-recorded fast path only returns the recorded subscription ID. | |
| +| First-purchase record | Requires exact intent consumption and no open refusal review, in the same transaction (`billingstore.go:766`). | |
| +| Recorded-subscription updates/renewals | Do not require consuming the original intent again. | |
| +| Operator `retire` | Archives/deletes only the specified account/intent and settles only its pending checkout quantity events (`billingstore.go:982`). | |
| +| Operator `keep` | Requires an owned completed bound session with a subscription. A remaining attributable purchase mismatch reopens the review; `keep` itself does not certify subscription correctness. | |
| +| Late review filing | Files nothing after the exact intent has gone. | |
| +| Retired subscription/session events | Account-scoped retirement checks settle them without recording; another account’s retirement does not match. | |
| + |
| +A first-record conflict rolls back state, assignment, event, entitlement and quantity-refresh writes. The operator’s lack of a service lock does not reopen the previously fixed recording-after-retirement race. |
| + |
| +Intent IDs are generated from 24 random bytes; there is no supported reuse operation. Recorded subscriptions skip the first-purchase retirement check. The fake’s distinct second session and late retired completion now assert unchanged account state and replacement binding (`billingcurrency_test.go:3045`, `:3063`); recording that completion would fail them. That helper still discards HTTP status, while the no-replacement completion separately asserts 204. |
| + |
| +**E/H/N/T — Barrier and rollout.** The trigger’s positive-span condition matches `AllowanceForMonth`: active non-Free assignment, bounded by month edges, paid-through endpoint and the next assignment across both tables, with revision ordering for the same second (`billingstore.go:208`; `internal/plans/month.go:146`). |
| + |
| +A same-second superseded book assignment contributes nothing. Free or inactive successors terminate preceding spans. Renewals and proven quantity changes alter `effective_at`; `period_end` remains the subscription’s paid-through endpoint copied by the assignment writer (`billingstore.go:627`, `:665`, `:740`). I found neither a valid-history version-1 escape nor an erroneous refusal of a legacy-only month. |
| + |
| +Startup creates the new trigger before retiring the old name. The retirement transaction checks the replacement exists; failure preserves the old trigger, and another start is idempotent (`billingstore.go:838`). Interruption can temporarily leave both, not neither. Older binaries remain unsupported after book rows exist: the barrier protects freezes, not their currency-unaware account writers. |
| + |
| +**F/G/J — Inclusions, validation and export.** Inclusions divide the pooled allowance by the quote’s charged quantity. Unavailable Personal uses its fallback quote’s own quantity (`billingpage.go:408`, `:427`). Current figures are exact for all permitted quantities; the divisibility and three-decimal checks establish why. `formatFigure(1,3)` truncates, but no current plan/band requires that approximation. Band-A sentences are pinned verbatim (`billingcurrency_test.go:2628`). |
| + |
| +The committed book has the required defaults and 36 prices; arithmetic checks cover overflow and half-up per-member rounding. Configuration admits complete additional catalogs, rejects partial catalogs and shared provider IDs (`cmd/console/billing.go:134`). Provisioning requires every selected-book key while allowing additional catalogs; its own-only `currency_options` rule agrees with the console (`implementation/carloku-paid-launch/stripe-provision.sh:426`, `:436`). |
| + |
| +The public export contains commercial definitions and country mappings, not customer data or credentials. Its current values match the site’s copied export. |
| + |
| +**X/CC — Quantity history.** All outcome-admission checks use `quantityOutcomesSQL`. Startup replaces a different processing-guard body transactionally (`billingquantityaudit.go:20`, `:38`). No healing sweep remains; pre-history and clock-rollback gaps survive restart. Resolution’s `checkout_refused_resolved` outcome is admitted consistently. |
| + |
| +**YY/ZZ — Outcome migration and rejected events.** Existing resolution rows read `retire` through the added column’s default. New writes normalize omitted outcomes to `retire`, preserving old-client behavior. Opposite-outcome replays conflict; Finding 2 affects exact `keep` replays. |
| + |
| +The rejected-events endpoint requires an operator CLI identity, returns at most 200 rows ordered by descending receipt time and then ID, and exposes event/object/subscription identifiers without provider payloads (`billingoperator.go:251`). It is not a complete refusal inventory: non-webhook refusals and retryable errors are not inserted there. Also, the sole production writer supplies account ID zero even for some refusals whose account was established (`billinghandlers.go:322`), so those rows omit the account sqid. |
| + |
| +**K/MM/QQ — Site.** Current annual prices, all currency/band slots, exact overage rates and band-scaled inclusions agree with the export. Country options carry the exported band and default currency. Links always contain both parameters; currency is populated only after an explicit supported URL/control choice (`site/src/site.js:75`). A locale-only display can differ from the console’s default because it deliberately sends no currency choice; the console supplies the purchase quote. |
| + |
| +Wrong literals in the checked slots fail equality checks. A literal equal to its slot’s current value passes, as the revised title now states. Numeric token counterexamples reject `10` inside `100` or `10,000`, and `250 MB` inside `1,250 MB`. Both draft escape hatches require exactly `"1"`; Finding 4 concerns which artifact they validate. |
| + |
| +## Claims the code contradicts |
| + |
| +- **“The resolution comments were corrected.”** Dispositions line 324 overstates this. `billingservice.go:294`, `:333` and `:1142` still say resolution retires the intent unconditionally. `billingstore.go:1020` also retains that implication. `keep` does otherwise. |
| +- **“Neither [operator door] calls the payment provider.”** `billingoperator.go:23` is now false: `billingKeepRefused` calls `GetCheckout` at `:236`. |
| +- **Unknown historical terms always become `ErrPriceTerms` and an invoice review.** Design line 143 overstates this. `internal/plans/month.go:182` returns `ErrUnknownTermsVersion` directly; `billinginvoice.go:320` reviews only `ErrPriceTerms`. Unknown terms can therefore remain a retry. |
| +- **Complete replay behavior is verified.** The outcome test establishes the tested opposite-outcome rejection, persistence and audit, but not exact `keep` replay after consumption. It also does not test upgrading an existing resolution table containing old rows. |
| +- **The rejected listing test establishes newest-first ordering and the bound.** It checks presence and basic fields, not order or the 200-row limit (`billingcurrency_test.go:2951`). It does not exercise account attribution. |
| +- **Mutation evidence:** review-three’s equivalent default-currency mutation and review-four’s exact-lookup/delete redundancy are now accurately qualified. Successful trigger migration does not demonstrate interruption behavior; provisioning assertions inspect script text rather than execute its verifier. Source cannot confirm historical mutation-run counts. |
| +- **Test dependencies:** the fake starts from wall-clock time (`billingservice_test.go:134`), as do authentication and webhook-signature cases. Cookie cases deliberately use fresh clients (`billingcurrency_test.go:1628`). I found no additional concrete failure caused by ordering, temporary-root selection or a shared cookie jar. |
| + |
| +## What must be verified against Stripe test mode before enablement |
| + |
| +1. **Catalog and checkout:** verify all 36 price identities, amounts, expanded currency-options shapes, tax settings and quantities. Complete USD/EUR/GBP purchases, including IN/GBP and Community GBP bands B/D. Session, subscription and item currencies must agree; Adaptive Pricing must remain disabled. |
| +2. **Ownership and expiry:** independently alter editable session/subscription metadata, exercise both event orders and the pass, and verify bound-ID expiry after ownership contradictions. Confirm expiry responses retain retirement-proof fields and expired URLs cannot accept payment. |
| +3. **Failure ordering:** exercise read failures, expiry failures, completion during expiry, and restart/pass activity between expiry and review persistence. No refused purchase may disappear without its review/retirement trail or permit an unintended replacement. |
| +4. **Resolution:** correct a purchase under an open review; activation must remain blocked. Exercise successful `keep`, refused expired-session `keep`, default `retire`, exact/opposite replays, and retired events beside a replacement intent. Each decision must be auditable and retired purchases must never activate. |
| +5. **Invoices and recovery:** create, recover, finalize and pay overage invoices in every currency. Verify all item/line currencies, exact allocations, floors, taxes, discounts, house waivers and legacy EUR request compatibility. |
| +6. **Subscription lifecycle:** verify quantity fencing, proration, renewal, cancellation and allowance dating. Test customer-currency restrictions with an ended subscription and outstanding invoice items. |
| + |
| +**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 fe44d155f..c37b50a9d 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -322,7 +322,26 @@ Reports: `OPUS-REVIEW-multi-currency-review16.md`, `CODEX-FINDINGS-multi-currenc |
| | Opus P3-2, Astra 1 (P3): the outcome was recorded nowhere, absent from the replay comparison, and absent from the audit. | Fixed: `billing_review_resolutions` gains an `outcome` column (added when missing; `retire` for the rows before it); the resolution writes it; a replay must repeat actor, note and outcome, else 409; the audit detail carries `outcome=`. The default is normalised to `retire` before anything is written. | `billingstore.go`, `billingoperator.go` | `TestACorrectedPurchaseWaitsForTheOperatorUnderItsReview` (the same resolution replayed with the other outcome is 409; the recorded outcome is `keep`); `TestKeepIsRefusedForAnIntentThatCannotRecord` (the audit) | |
| | Opus claim 2: the later retirement check in reconcile was unreachable with a retirement. | Removed; the early check is the one. | `billingservice.go` | | |
| | Opus claims 1 and 3, Astra claims: the site test's title claimed literal provenance; the design's `keep` sentence; the unknown-terms row's "known here"; the review-four row's lookup-history sentence; four "every resolution retires" comments. | Corrected: the site test's title and comment say output equality, not provenance (site `c12318d`); the design states the `keep` condition and the recorded outcome; the rows and comments say retire or keep. | this file, the design, `billingservice.go`, `billingstore.go`, site | | |
| -| Named missing since round eleven by both: a listing of the webhook events the console refused. | Built: `GET /api/billing/rejected` (operator only), newest first, at most 200, with the event's type, object, subscription and account where known. | `billingoperator.go` `handleAPIBillingRejected`, `server.go` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` (the settled events after a resolution are listed); `TestBillingOperatorDoorsNeedTheOperatorBit` (the member is refused) | |
| +| Named missing since round eleven by both: a listing of the webhook events the console refused. | Built: `GET /api/billing/rejected` (operator only), newest first, at most 200, with the event's type, object, subscription and, from the seventeenth-round commit, the account where the refusal knew it (a retired intent's event); the webhook's other identity refusals know none. | `billingoperator.go` `handleAPIBillingRejected`, `server.go` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` (the settled events after a resolution are listed); `TestBillingOperatorDoorsAnswerOnlyOperators` (the member is refused) | |
| | The provisioning script's step-2 notes said "no currency_options, ever". | Aligned with the rule the console and `--check` apply: never pass it; an expanded read showing the price's own currency at its own amount passes; any other entry is refused. | `stripe-provision.sh` | `internal/plans` (the script's text) | |
| Open, as both list: everything that needs Stripe test mode (both lists, with `keep` against a corrected purchase, a hand-made subscription's `keep` refused, and `retire` against `keep` on one purchase added this round), the two unreviewed system strings, the operator's door for a quantity-history gap. The seventeenth review reads this commit. |
| + |
| +## Review seventeen (reviewed `db130cfc`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review17.md`, `CODEX-FINDINGS-multi-currency-review17.md`. Both found review sixteen's findings closed with tests that fail without each fix. Astra rejected on one P2 in the refusal's own ordering; Opus's four P3s and Astra's four P3s overlap on the `keep` door and the rejected-events listing, and Astra's two site findings are fixed on the site branch. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Astra 1 (P2): a pass can retire the intent between the refusal's expiry and the filing of its review. Both sites that expire a bound session (reconcile's refusal of a first purchase; `ownBoundSession` under every reader) expired first and filed second, so a pass that read the expired session in between, or a crash between the two, retired the intent with no record. | Fixed: the review is filed first, then the session expired, at both sites. With the review open no ordinary retirement can take the intent (`retireIntent`'s open-review clause). An expiry that fails leaves the review standing and is tried again at the next read (the open review files nothing new). | `billingservice.go` `ownBoundSession`, reconcile's `refuse()` | `TestNoPassRetiresTheIntentBetweenExpiryAndReview` (two paths; the fake's `expireHook` runs the store's expiry-with-proof retirement inside the provider's expiry): with either site's order put back, that path's retirement succeeds and the test fails. | |
| +| Opus P3-1, Astra 2: an exact `keep` replay after the corrected purchase recorded answered 409 (the gate ran before the replay branch), and a replay with the provider unreachable 503. | Fixed: the handler reads the review first; an occurrence already resolved skips the gate and goes to the store's replay comparison (200 for the exact replay, 409 for a different note or outcome). | `billingoperator.go` | `TestACorrectedPurchaseWaitsForTheOperatorUnderItsReview` (the identical `keep` replay answers 200 after the record). | |
| +| Opus P3-2, Astra 3: `keep` dereferenced a nil billing service when billing is off. | Fixed: 409 "billing is not configured; resolve with retire" before the gate. | `billingoperator.go` | `TestKeepIsRefusedForAnIntentThatCannotRecord`. | |
| +| Opus P3-3: `keep` was accepted for a completed session that is not the intent's purchase (wrong currency), which could never record and would re-file. | Fixed: the gate also requires `intentSession`; otherwise 409. | `billingoperator.go` `billingKeepRefused` | `TestKeepIsRefusedForAnIntentThatCannotRecord` (second fixture: a paid session in the wrong currency; `keep` 409). | |
| +| Opus P3-4, Astra claim: the rejected-events listing never carried an account (the only writer passed 0), ordered ties oldest first, and the test checked neither order, account nor the bound. | Fixed: reconcile's retired-settle returns a typed identity error carrying the account (`billingIdentityOfAccount`), the webhook records it; `ORDER BY received_at DESC, rowid DESC`; the test checks newest first and the account on every settled row, and a new test the 200-row bound. The listing's coverage stays what it was: the webhook's refusals (an event refused for being nobody's, and a retired intent's settled events); the refusals that are reviews are listed by the reviews door. | `billingoperator.go`, `billinghandlers.go`, `billingservice.go` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution`, `TestTheRejectedListingIsBoundedAndNewestFirst`. The first commit of this fix looked the account up under the open rows and deadlocked the store's one connection (the test hung); the rows are read to the end first. | |
| +| Astra 4 (P3, site): `build` and `check` validated whatever page was on disk, so a stale page could be published past a newer export. | Fixed on the site: both render from the export first, then check, then eleventy; a test pins the order. | site `package.json`, `test/pricing-pricebook.test.mjs` | site `4026231`. | |
| +| Astra 5 (P3, site): a fractional-cent floor rendered as `$1.20.5`, and the test repeated the arithmetic. | Fixed on the site: `money()` refuses a minor amount that is not whole; the test asserts the refusal and that every floor in the book is whole cents. | site `copy-review/pricing/build.mjs` | site `4026231`. | |
| +| Astra claim: an existing resolutions table with rows from before the `outcome` column is not tested. | Added: a store opened on a database holding the older table and a row migrates the column with `retire` as the row's outcome, and a `keep` replay against it is refused as a different outcome. | `billingstore.go` migration | `TestAnOlderResolutionRowReadsAsRetire`. | |
| +| Claims (both): `billingservice.go` comments at the review-keyed refusal, the reconcile retirement and the operator's resolution still said the resolution retires unconditionally; `billingstore.go`'s filing comment likewise; `billingoperator.go` "neither door calls the payment provider" (`keep` reads the bound session); design §allowance said unknown terms become `ErrPriceTerms` and a review (`month.go` returns `ErrUnknownTermsVersion` and the invoice engine retries it). | Corrected in place. | those files, the design | | |
| +| Astra claims on evidence: historical mutation counts cannot be confirmed from source; provisioning assertions read the script's text; the fake starts from wall-clock time. | Stated as they are: the mutation runs are the author's, named per round here; the provisioning check is exercised against the live catalog only in the test-mode rehearsal (approval E); no concrete ordering or clock failure was found by either reviewer or the author. | | | |
| + |
| +Open, as both list: everything that needs Stripe test mode (Astra's six-part list, this round's additions: expiry failing after the review is filed, a restart between the two, `keep` against a corrected purchase at the provider), the two unreviewed system strings, the operator's door for a quantity-history gap, and the metered model's remaining code (READINESS on the launch branch). |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review17.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review17.md| new file mode 100644 |
| index 000000000..56c9506fa |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review17.md |
| @@ -0,0 +1,109 @@ |
| +# Seventeenth review of `feat/carloku-multi-currency-20261001` at `db130cfc` (base `12e70d95`, delta from `3527a066`) |
| + |
| +**Verdict: accept with fixes.** Every finding from the sixteenth review is closed, and each has a test that would fail without its fix. The new `keep` gate and the outcome column bring four P3s and some stale comments. The delta does not touch money computation, Stripe parameters, the quote signature, the trigger or the pages. |
| + |
| +I read source with `git show` only, plus the site at `c12318d`. I ran nothing and modified nothing. |
| + |
| +## The sixteenth review's findings |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1**: `keep` was accepted for an intent that can never record | **Closed.** `billingKeepRefused` (`billingoperator.go:225`) reads the bound session by its bound id. It accepts `keep` only if `ownSession` holds (mode, accounts, intent, customer, bound id), the status is `complete` and a subscription is set; otherwise it answers 409. The review stays open because the 409 comes before the store is called. | **No.** In `TestKeepIsRefusedForAnIntentThatCannotRecord`, without the gate `keep` resolves with 200 and the open-review count becomes 0. | |
| +| **Opus P3-2 / Astra 1**: the outcome was recorded nowhere and was missing from the replay comparison | **Closed.** There is now an `outcome` column, added when missing with default `retire`. The insert writes `r.outcome()`. A replay compares actor, note and outcome (`billingstore.go:948`). The audit detail carries `outcome=`. The default is normalised before the write. | **No.** Without the outcome comparison, the opposite-outcome replay answers 200, not 409. Without the column, the recorded-outcome query errors. Without the audit change, the `outcome=retire` check fails. | |
| +| **Opus claim 1** (site title and comment claimed provenance) | **Corrected** at site `c12318d`. The title and comment now say "output equality". | n/a | |
| +| **Opus claim 2** (later retirement check unreachable) | **Removed** (`billingservice.go:889-893`). The early check at `:801` is the only one, and the 503 for "no intent, not retired" is unchanged. | n/a | |
| +| **Opus claim 3** (design and store comment on `keep`) | **Design and store comment corrected.** `billingservice.go` comments are not; see claims below. | n/a | |
| +| **Astra claims**: lookup history, "terms known", four "every resolution retires" comments | **Partly corrected.** The review-four row now says why no test can build the lookup-distinguishing history, and that is right: filing refuses a retired intent. The unknown-terms row is corrected. **Three of the four comments still say resolution retires** (claims below). | n/a | |
| + |
| +**Where the reviewers disagreed.** Astra rated the outcome finding P3 for its replay identity. Opus rated it P3 for its traceability. Both readings were right and the fix covers both, so this was a disagreement about emphasis, not severity. |
| + |
| +## Findings |
| + |
| +### P3-1. An identical `keep` replay after the corrected purchase records answers 409, and the message points the operator to a `retire` that also answers 409 |
| + |
| +- **Where:** `billingoperator.go:315-324`. The `keep` gate runs before `resolveBillingReview`'s replay branch (`billingstore.go:938-950`). |
| +- **Scenario:** |
| + 1. The operator resolves occurrence 1 with `keep`, and the corrected subscription records. The record consumes the intent. |
| + 2. The operator's client retries the identical request after a lost response. `billingKeepRefused` finds no standing intent and answers **409 "keep needs a completed session … (no such intent stands); resolve with retire"**. |
| + 3. Following that advice, `retire` hits the outcome mismatch and also answers **409**. |
| +- **Also:** an identical replay while Stripe is unreachable answers 503 instead of the 200 the store promises. |
| +- **Why it matters:** the store's documented contract ("repeating the exact resolution … succeeds without writing") is broken for `keep` only, and the operator is told to do the wrong thing. No money moves. |
| +- **Fix:** run the gate only when the review occurrence is still open. Either read `resolved_at` first and skip the gate when it is set, or move the gate's decision into the store call after the replay branch. Add a test: an identical `keep` replay after the record answers 200. |
| + |
| +### P3-2. `keep` dereferences a nil billing service when billing is off |
| + |
| +- **Where:** `billingoperator.go:236` (`s.billing.client.GetCheckout`). `billingOperator` (`:158`) does not check `s.billing`, and the webhook shows `s.billing` is nil when billing is off (`billinghandlers.go:236`). |
| +- **Scenario:** a console that once ran billing has an open checkout refusal, and billing is then turned off. An operator posts `outcome:"keep"`. The handler panics; `net/http` recovers and drops the connection. Nothing is resolved and nothing is corrupted, but the door crashes instead of answering. |
| +- **Fix:** if `s.billing == nil`, answer 409 or 503 for `keep` ("billing is not configured; resolve with retire"). |
| + |
| +### P3-3. `keep` is still accepted for a completed session that can never record |
| + |
| +- **Where:** `billingKeepRefused` (`billingoperator.go:240-246`) checks ownership, `complete` and a subscription, but not `intentSession`. |
| +- **Scenario:** |
| + 1. A session is paid in EUR on a GBP intent. The binder files the refusal review. |
| + 2. The session is complete, owned and has a subscription, so `keep` is accepted. |
| + 3. A completed session's currency and amount cannot be corrected at the provider, so every later read fails `intentSession`. |
| + 4. The next pass (`billingpass.go`, the `complete` branch) refiles the review as occurrence 2 (`fileCheckoutRefusedReview`, `billingstore.go:1042`). |
| +- **Effect:** visible, not silent. The hold comes back and the operator must then retire. But the door accepted an outcome its own comment says it refuses ("any other kept intent could never record"). |
| +- **Fix:** also require `s.billing.intentSession(session, intent, acct)` in `billingKeepRefused`, and answer 409 "the session itself is not the purchase; retire" otherwise. Add a test: a wrong-currency paid session, then `keep`, which must be 409. |
| + |
| +### P3-4. The rejected-events listing never carries an account, sorts ties oldest-first, and covers only the webhook's identity refusals |
| + |
| +- **Where:** `billinghandlers.go:322` (the only writer passes `accountID` 0) and `billingoperator.go:255` (the listing). |
| +- **Scenario:** a retired intent's subscription event is settled. Reconcile knew the account (`billingIntentRetired(acct.ID, …)`), but the row stores 0, so `account` is never present in any row. Dispositions say "account where known"; it is never known. |
| +- **Ordering:** `ORDER BY received_at DESC, id` lists events in the same second in id order, not newest first. |
| +- **Coverage:** these refusals are in no list anywhere: |
| + - identity refusals reached through the pass or the success URL; |
| + - every "retry for ever" outcome Stripe eventually abandons (a subscription naming no intent and no retirement → `sql.ErrNoRows`; unknown terms with no standing intent). |
| +- **Access and disclosure are fine:** the listing is operator-only (404 otherwise), returns at most 200 rows, and holds Stripe object, subscription and event ids only, with no payload and no PII. |
| +- **Fix:** either return the account from the reconcile paths that resolved it (a typed identity error carrying `acct.ID`) and pass it to `rejectBillingEvent`, or drop the field and the "where known" claim. Order ties by `rowid DESC`. State the uncovered refusals in the open list. |
| + |
| +## The added sections |
| + |
| +**XX. `billingKeepRefused`.** |
| +- **The read is the console's own bound session.** It reads `intent.SessionID` from the standing intent, and that intent must be the review's intent (`intent.ID != intentID` → 409). `ownSession` includes the bound id, so an answer carrying another id is refused. |
| +- **No race makes `keep` unsafe:** |
| + - `complete` is terminal at the provider. |
| + - The pass cannot retire the intent while the review is open: `retireIntent` carries the open-review clause. |
| + - A recurrence between the gate and the store makes the store answer stale (409). |
| +- **Remaining gaps:** `keep` can still leave a standing intent that cannot record only as P3-3, and that case re-files visibly. |
| +- **409 and the review:** 409 is the right answer, and the review stays open because the store is not called. The tested case confirms both. |
| +- **The replay path is wrong** (P3-1). |
| + |
| +**YY. The outcome column.** |
| +- **Migration:** `addColumnIfMissing … DEFAULT 'retire'` runs after the `CREATE TABLE`, so fresh and existing databases converge. `ALTER TABLE ADD COLUMN` does not fire the no-update trigger. Earlier rows read `retire`, which is correct for every build except `3527a066`, whose `keep` resolutions would be mislabelled. That build was never deployed with billing on, so this is moot. |
| +- **Replay:** the comparison is exact. |
| +- **Audit:** the detail carries the outcome only for checkout refusals. |
| +- **An old operator client** (no outcome field) gets `retire`, exactly as before `keep` existed, and an old client's replay of a `keep` answers 409. |
| +- **Minor:** `keep` sent for any other reason is stored as `keep` and does nothing. A later default replay of it answers 409, which is honest. |
| + |
| +**ZZ. `GET /api/billing/rejected`.** Covered in P3-4. |
| + |
| +**Everything else (A–WW) is unchanged by this delta.** |
| +- The GBP purchase from `?country=IN¤cy=gbp` still has no conversion or re-pricing anywhere in its chain. |
| +- The signature, the trigger, `ownSession` on every path and the record's open-review clause stand as rounds fifteen and sixteen traced them. |
| +- The removed later retirement check leaves only `sql.ErrNoRows` (a retry) for "no intent stands, not retired", which is the behaviour the early check was built to precede. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **Dispositions, review sixteen, last row ("four 'every resolution retires' comments … say retire or keep").** Three still say resolution retires: |
| + - `billingservice.go:294` ("until the operator resolves the review, which retires it"); |
| + - `:333` ("resolving it retires the intent, so it does not come back"); after a `keep` on a paid wrong-currency session it does come back (P3-3); |
| + - `:1141-1142` ("which retires exactly that intent"). |
| +2. **`billingstore.go:905-906`** still says a replay with "a different actor or note" is refused. The outcome is now part of that comparison. **`billingoperator.go:358`** answers "already resolved with a different note" to an opposite-outcome replay. |
| +3. **`billingoperator.go:226-229` and the resolution's store comment** say `keep` is accepted only for an intent that can still record. P3-3 is the exception. |
| +4. **Dispositions, review sixteen ("with the event's … account where known").** The account is never stored (P3-4). |
| +5. **`acknowledgeBillingRejection` (`billinghandlers.go:315-318`)** says the account is "by definition, not resolvable here". For a retired intent's settlement, reconcile resolved it. |
| + |
| +I found no contradiction in this round's two mutation claims: removing the gate fails the 409 assertion, and dropping the outcome from the replay fails the opposite-outcome 409. The review-four row's new sentence matches the code: `fileCheckoutRefusedReview` (`billingstore.go:1035`) refuses to file for an intent that is gone. |
| + |
| +## Verify in Stripe test mode before enabling |
| + |
| +All earlier lists stand (catalog identity for 36 prices, three-currency purchases including IN/GBP, event orderings, bound-session expiry, review lifecycle, overage invoices, quantity and subscription lifecycle). Add: |
| + |
| +1. **`keep` on a corrected subscription whose session completed.** Expect 200, then the next delivery or pass records. Then replay the identical `keep`: expect 200 once P3-1 is fixed (409 today). |
| +2. **`keep` on a session paid in the wrong currency.** Expect 409 after P3-3's fix. Today it is accepted and the review re-files on the next pass. |
| +3. **`keep` while `GetCheckout` fails** (bad key or network): 503, the review still open, nothing written. |
| +4. **`GET /api/billing/rejected` after a retired subscription's events.** Confirm the rows, their ids and the account field once P3-4 is decided. |
| + |
| +**Verdict: accept with fixes** (P3-1 to P3-4 and claims 1, 2, 4 and 5; claim 3 is resolved by P3-3's fix). |
diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go| index 31caa8236..ceabc831c 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -2953,6 +2953,12 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| if status != http.StatusOK || len(rejected) < 3 { |
| t.Fatalf("the rejected events listing: %d %v; want the settled events listed", status, body) |
| } |
| + // Newest first: the first row's receipt is no earlier than the last's. |
| + first, _ := rejected[0].(map[string]any) |
| + last, _ := rejected[len(rejected)-1].(map[string]any) |
| + if first["received_at"].(float64) < last["received_at"].(float64) { |
| + t.Fatalf("the listing is not newest first: %v ... %v", first, last) |
| + } |
| seen := map[string]bool{} |
| for _, r := range rejected { |
| row, _ := r.(map[string]any) |
| @@ -2960,6 +2966,11 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| if row["reason"] != "billing_identity_mismatch" || row["type"] == "" || row["object"] == "" { |
| t.Fatalf("a rejected event row: %v", row) |
| } |
| + // A retired intent's event was settled by an account the refusal |
| + // knew: the row names it (review seventeen, Opus P3-4). |
| + if strings.HasPrefix(row["id"].(string), "evt_after_resolution_") && row["account"] != acct.Sqid { |
| + t.Fatalf("a settled event's account: %v; want %s", row, acct.Sqid) |
| + } |
| } |
| for _, id := range []string{"evt_after_resolution_0", "evt_after_resolution_1", "evt_after_resolution_done"} { |
| if !seen[id] { |
| @@ -3951,6 +3962,12 @@ func TestACorrectedPurchaseWaitsForTheOperatorUnderItsReview(t *testing.T) { |
| if _, err := fx.st.billingCheckoutIntent(acct.ID); err == nil { |
| t.Fatal("the intent stands after the record consumed it") |
| } |
| + // The identical keep replayed after the record (a lost response): |
| + // repeated without writing, 200 (review seventeen, Opus P3-1). |
| + status, body = postJSON(t, fx.ts.URL+"/api/billing/reviews/resolve", map[string]any{"account": acct.Sqid, "subscription": intent.ID, "reason": billingCheckoutRefusedReason, "occurrences": 1, "note": "corrected at the provider", "outcome": "keep"}, token) |
| + if status != http.StatusOK || body["resolved"] != true { |
| + t.Fatalf("the identical keep replayed after the record: %d %v; want 200", status, body) |
| + } |
| // An outcome that is neither is refused; the same resolution replayed |
| // with the other outcome is refused too, and the outcome is in the |
| // resolution's record. |
| @@ -3994,6 +4011,25 @@ func TestKeepIsRefusedForAnIntentThatCannotRecord(t *testing.T) { |
| if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=? AND resolved_at=0`, acct.ID, intent.ID, billingCheckoutRefusedReason).Scan(&open); err != nil || open != 1 { |
| t.Fatalf("%d open reviews (%v); want the review still open", open, err) |
| } |
| + // A session paid in the wrong currency is complete with a subscription |
| + // and its own, yet never the purchase the intent describes: keep is |
| + // refused for it too (review seventeen, Opus P3-3). |
| + fx2, f2, acct2 := newCurrencyFixture(t) |
| + f2.customerCountry = "GB" |
| + if dest, err := fx2.srv.startBillingCheckout(context.Background(), acct2, plans.PlanPersonal, "GB", "", "", f2.now); err != nil || dest == "" { |
| + t.Fatalf("second fixture's click: dest %q err %v", dest, err) |
| + } |
| + intent2, _ := fx2.st.billingCheckoutIntent(acct2.ID) |
| + f2.mu.Lock() |
| + f2.sessionStatus, f2.sessionCurrency, f2.subscriptionPrice = "complete", "eur", intent2.Price |
| + f2.mu.Unlock() |
| + if code := f2.webhookPost(t, fx2, "evt_keep_wrong_currency", "checkout.session.completed", intent2.SessionID); code != http.StatusNoContent { |
| + t.Fatalf("the wrong-currency completion: %d", code) |
| + } |
| + token2 := billingOperatorToken(t, fx2, "ops@keymail.dev", "code-billing-op-keep-2") |
| + if status, body := postJSON(t, fx2.ts.URL+"/api/billing/reviews/resolve", map[string]any{"account": acct2.Sqid, "subscription": intent2.ID, "reason": billingCheckoutRefusedReason, "occurrences": 1, "note": "x", "outcome": "keep"}, token2); status != http.StatusConflict { |
| + t.Fatalf("keep on a session paid in the wrong currency: %d %v; want 409", status, body) |
| + } |
| // retire works, and the audit names the outcome. |
| status, body = postJSON(t, fx.ts.URL+"/api/billing/reviews/resolve", map[string]any{"account": acct.Sqid, "subscription": intent.ID, "reason": billingCheckoutRefusedReason, "occurrences": 1, "note": "refunded"}, token) |
| if status != http.StatusOK || body["retired"] != true { |
| @@ -4062,3 +4098,142 @@ func TestARetiredSubscriptionSettlesBesideAReplacementIntent(t *testing.T) { |
| t.Fatalf("the replacement intent: %+v %v; want it untouched", after, err) |
| } |
| } |
| + |
| +// Between the provider's expiry of the bound session and the filing of the |
| +// review, nothing may retire the intent: the review is filed first on both |
| +// paths that expire a bound session (reconcile's refusal of a first |
| +// purchase, and ownBoundSession under every reader), so a pass that meets |
| +// the expired session in that interval (here: the store's retirement with |
| +// the expired proof, which is what the pass's expiry branch does) finds |
| +// the review open and retires nothing. (Review seventeen, Astra 1, P2.) |
| +func TestNoPassRetiresTheIntentBetweenExpiryAndReview(t *testing.T) { |
| + check := func(t *testing.T, fx *consoleFixture, acct *Account, intent billingIntent, retired chan error) { |
| + t.Helper() |
| + select { |
| + case err := <-retired: |
| + if !errors.Is(err, errBillingConflict) { |
| + t.Fatalf("a retirement during the refusal's expiry: %v; want a conflict (the review is already open)", err) |
| + } |
| + default: |
| + t.Fatal("fixture: the expiry hook never ran") |
| + } |
| + if standing, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || standing.ID != intent.ID { |
| + t.Fatalf("intent %+v %v; want it standing under its review", standing, err) |
| + } |
| + var open int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=? AND resolved_at=0`, acct.ID, intent.ID, billingCheckoutRefusedReason).Scan(&open); err != nil || open != 1 { |
| + t.Fatalf("%d open reviews (%v); want one", open, err) |
| + } |
| + } |
| + start := func(t *testing.T) (*consoleFixture, *fakeBillingStripe, *Account, billingIntent, *billingstripe.Session, chan error) { |
| + t.Helper() |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + intent, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + proof := &billingstripe.Session{ID: intent.SessionID, Status: "expired", LiveMode: false, ClientReferenceID: acct.Sqid, CustomerID: "cus_paid", Metadata: map[string]string{"checkout_intent": intent.ID, "account_sqid": acct.Sqid}} |
| + retired := make(chan error, 1) |
| + f.mu.Lock() |
| + f.expireHook = func() { |
| + // The pass's expiry branch, at the instant the provider has |
| + // expired the session and the console has not yet returned |
| + // from refusing. |
| + retired <- fx.st.expireBillingCheckout(intent, proof, f.now.Add(time.Minute)) |
| + } |
| + f.mu.Unlock() |
| + return fx, f, acct, intent, proof, retired |
| + } |
| + t.Run("reconcile's refusal of a first purchase", func(t *testing.T) { |
| + fx, f, acct, intent, _, retired := start(t) |
| + f.mu.Lock() |
| + // A hand-made subscription at another band's price; the session open. |
| + f.subscriptionPrice = currencyPriceID(plans.PlanPersonal, plans.BandD, plans.GBP) |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_race_expiry", "customer.subscription.created", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the hand-made subscription event: %d", code) |
| + } |
| + check(t, fx, acct, intent, retired) |
| + }) |
| + t.Run("ownBoundSession, under the pass's reader", func(t *testing.T) { |
| + fx, f, acct, intent, _, retired := start(t) |
| + f.mu.Lock() |
| + // The bound session, still open, reads back as another customer's. |
| + f.sessionCustomer = "cus_other" |
| + f.mu.Unlock() |
| + _ = fx.srv.billingPass(context.Background(), f.now.Add(time.Minute)) |
| + if f.expireCalls != 1 { |
| + t.Fatalf("%d expiries; want the pass's reader to expire the open bound session", f.expireCalls) |
| + } |
| + check(t, fx, acct, intent, retired) |
| + }) |
| +} |
| + |
| +// The rejected-events listing is bounded at 200 rows, newest first, and |
| +// names the account where the refusal knew it. (Review seventeen, Opus |
| +// P3-4 and Astra's claim that neither was tested.) |
| +func TestTheRejectedListingIsBoundedAndNewestFirst(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + for i := 0; i <= 200; i++ { |
| + if err := fx.st.rejectBillingEvent(fmt.Sprintf("evt_bulk_%03d", i), "customer.subscription.updated", "sub_bulk", "sub_bulk", acct.ID, f.now.Add(time.Duration(i)*time.Second)); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + token := billingOperatorToken(t, fx, "ops@keymail.dev", "code-billing-op-bound") |
| + status, body := getJSON(t, fx.ts.URL+"/api/billing/rejected", token) |
| + rejected, _ := body["rejected"].([]any) |
| + if status != http.StatusOK || len(rejected) != 200 { |
| + t.Fatalf("%d rows, status %d; want the listing bounded at 200", len(rejected), status) |
| + } |
| + first, _ := rejected[0].(map[string]any) |
| + last, _ := rejected[199].(map[string]any) |
| + if first["id"] != "evt_bulk_200" || last["id"] != "evt_bulk_001" || first["account"] != acct.Sqid { |
| + t.Fatalf("first %v last %v; want the newest 200, newest first, each naming the account", first, last) |
| + } |
| +} |
| + |
| +// A resolutions table from before the outcome column, with rows in it, |
| +// gains the column at open, and an old row reads as the one outcome a |
| +// resolution could have then: retire. A keep replayed against it is a |
| +// different resolution and is refused. (Review seventeen, Astra's claim |
| +// that the migration of an existing table was not tested.) |
| +func TestAnOlderResolutionRowReadsAsRetire(t *testing.T) { |
| + path := t.TempDir() + "/console.db" |
| + old, err := Open(path) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + // The older build's table: the same, without the column. |
| + for _, q := range []string{ |
| + `ALTER TABLE billing_review_resolutions DROP COLUMN outcome`, |
| + `INSERT INTO billing_reviews(account_id,subscription,reason,created_at,occurrences,last_seen_at,resolved_at) VALUES(7,'ci_old',?,100,1,100,200)`, |
| + `INSERT INTO billing_review_resolutions(account_id,subscription,reason,occurrence,last_seen_at,actor,note,resolved_at) VALUES(7,'ci_old',?,1,100,'paul@keymail.dev','refunded',200)`, |
| + } { |
| + if _, err := old.db.Exec(q, billingCheckoutRefusedReason); err != nil { |
| + t.Fatalf("%s: %v", q, err) |
| + } |
| + } |
| + old.Close() |
| + s, err := Open(path) |
| + if err != nil { |
| + t.Fatalf("open on the older table: %v", err) |
| + } |
| + defer s.Close() |
| + if err := s.SetOperator("paul@keymail.dev", true); err != nil { |
| + t.Fatal(err) |
| + } |
| + var outcome string |
| + if err := s.db.QueryRow(`SELECT outcome FROM billing_review_resolutions WHERE account_id=7 AND subscription='ci_old'`).Scan(&outcome); err != nil || outcome != "retire" { |
| + t.Fatalf("the old row's outcome: %q %v; want retire", outcome, err) |
| + } |
| + base := billingReviewResolution{AccountID: 7, Subscription: "ci_old", Reason: billingCheckoutRefusedReason, Occurrence: 1, Actor: "paul@keymail.dev", Note: "refunded"} |
| + if err := s.resolveBillingReview(base, time.Unix(300, 0)); err != nil { |
| + t.Fatalf("the exact replay of the old resolution: %v; want it to pass without writing", err) |
| + } |
| + keep := base |
| + keep.Outcome = "keep" |
| + if err := s.resolveBillingReview(keep, time.Unix(300, 0)); !errors.Is(err, errBillingReviewResolved) { |
| + t.Fatalf("keep against the old retire: %v; want it refused as a different resolution", err) |
| + } |
| +} |
diff --git a/internal/console/billinghandlers.go b/internal/console/billinghandlers.go| index 5e367884a..1133845d4 100644 |
| --- a/internal/console/billinghandlers.go |
| +++ b/internal/console/billinghandlers.go |
| @@ -315,11 +315,17 @@ func (s *Server) acknowledgeBillingRejection(w http.ResponseWriter, event billin |
| if !errors.Is(err, errBillingIdentity) { |
| return false |
| } |
| - // The account is, by definition, not resolvable here (that is why the |
| - // event was rejected), so 0 is recorded for it; the object and |
| - // subscription ids are what a human uses to investigate within Stripe's |
| - // 30-day event window. |
| - if err := s.store.rejectBillingEvent(event.ID, event.Type, objectID, subscriptionID, 0, time.Now()); err != nil { |
| + // The account is recorded where the refusal knew it (a retired |
| + // intent's event is settled by the account the subscription names); |
| + // otherwise it is not resolvable here, and 0 is recorded. The object |
| + // and subscription ids are what a human uses to investigate within |
| + // Stripe's 30-day event window. |
| + var accountID int64 |
| + var known *billingIdentityOfAccount |
| + if errors.As(err, &known) { |
| + accountID = known.AccountID |
| + } |
| + if err := s.store.rejectBillingEvent(event.ID, event.Type, objectID, subscriptionID, accountID, time.Now()); err != nil { |
| return false |
| } |
| log.Printf("console: billing webhook identity rejected; event recorded for review") |
diff --git a/internal/console/billingoperator.go b/internal/console/billingoperator.go| index 37294af3c..12966dff4 100644 |
| --- a/internal/console/billingoperator.go |
| +++ b/internal/console/billingoperator.go |
| @@ -18,9 +18,11 @@ import ( |
| // a billing review hold (resolveBillingReview had no caller, so every hold |
| // was permanent) and marking an account as a house account. |
| // |
| -// Both are API doors behind the CLI token of an operator (users.operator), |
| -// answering 404 to everyone else so they confirm nothing. Neither calls the |
| -// payment provider. Both leave an append-only record of who did what. |
| +// All are API doors behind the CLI token of an operator (users.operator), |
| +// answering 404 to everyone else so they confirm nothing. One call reaches |
| +// the payment provider: a resolution with the outcome keep reads the |
| +// intent's bound session to see that it can still record. Each writing |
| +// door leaves an append-only record of who did what. |
| // |
| // GET /api/billing/reviews open review holds |
| // POST /api/billing/reviews/resolve clear one hold, conditional on its occurrence count |
| @@ -220,8 +222,8 @@ type billingRejectedEventRow struct { |
| // billingKeepRefused says why a checkout refusal cannot be resolved with |
| // keep: the intent is gone, unbound, or its bound session is not its own, |
| -// not complete, or carries no subscription. An empty answer means keep is |
| -// allowed. |
| +// not the purchase the intent describes, not complete, or carries no |
| +// subscription. An empty answer means keep is allowed. |
| func (s *Server) billingKeepRefused(ctx context.Context, acct *Account, intentID string) (string, error) { |
| intent, err := s.store.billingCheckoutIntent(acct.ID) |
| if errors.Is(err, sql.ErrNoRows) || (err == nil && intent.ID != intentID) { |
| @@ -240,6 +242,9 @@ func (s *Server) billingKeepRefused(ctx context.Context, acct *Account, intentID |
| if !s.billing.ownSession(session, intent, acct) { |
| return "the session read back is not the intent's own", nil |
| } |
| + if !s.billing.intentSession(session, intent, acct) { |
| + return "the session itself is not the purchase the intent describes (its plan, band, terms or currency), which no correction at the provider changes", nil |
| + } |
| if session.Status != "complete" || session.SubscriptionID == "" { |
| return "the session is " + session.Status + " with subscription " + strconv.Quote(session.SubscriptionID), nil |
| } |
| @@ -252,32 +257,41 @@ func (s *Server) handleAPIBillingRejected(w http.ResponseWriter, r *http.Request |
| if _, ok := s.billingOperator(w, r); !ok { |
| return |
| } |
| - rows, err := s.store.db.Query(`SELECT id,event_type,reason,object_id,subscription_id,account_id,received_at FROM billing_rejected_events ORDER BY received_at DESC, id LIMIT 200`) |
| + rows, err := s.store.db.Query(`SELECT id,event_type,reason,object_id,subscription_id,account_id,received_at FROM billing_rejected_events ORDER BY received_at DESC, rowid DESC LIMIT 200`) |
| if err != nil { |
| log.Printf("console: api billing rejected: %v", err) |
| jsonError(w, http.StatusInternalServerError, "could not read rejected events. try again") |
| return |
| } |
| - defer rows.Close() |
| + // The rows are read to the end before any account is looked up: the |
| + // store has one connection, and a second query while these rows are |
| + // open would wait for it for ever. |
| out := make([]billingRejectedEventRow, 0) |
| + var accountIDs []int64 |
| for rows.Next() { |
| var x billingRejectedEventRow |
| var accountID int64 |
| if err := rows.Scan(&x.ID, &x.Type, &x.Reason, &x.Object, &x.Subscription, &accountID, &x.ReceivedAt); err != nil { |
| + rows.Close() |
| jsonError(w, http.StatusInternalServerError, "could not read rejected events. try again") |
| return |
| } |
| - if accountID != 0 { |
| - if acct, err := s.store.AccountByID(accountID); err == nil && acct != nil { |
| - x.Account = acct.Sqid |
| - } |
| - } |
| out = append(out, x) |
| + accountIDs = append(accountIDs, accountID) |
| } |
| - if err := rows.Err(); err != nil { |
| + err = rows.Err() |
| + rows.Close() |
| + if err != nil { |
| jsonError(w, http.StatusInternalServerError, "could not read rejected events. try again") |
| return |
| } |
| + for i, id := range accountIDs { |
| + if id != 0 { |
| + if acct, err := s.store.AccountByID(id); err == nil && acct != nil { |
| + out[i].Account = acct.Sqid |
| + } |
| + } |
| + } |
| writeJSON(w, http.StatusOK, map[string]any{"rejected": out}) |
| } |
| @@ -309,11 +323,25 @@ func (s *Server) handleAPIBillingReviewResolve(w http.ResponseWriter, r *http.Re |
| } |
| if req.Outcome == "keep" && req.Reason == billingCheckoutRefusedReason { |
| // Keeping an intent is for a purchase that can still record: the |
| - // intent's bound session, read now, must be its own, complete and |
| - // carrying a subscription. Any other kept intent could never |
| - // record and would later expire with no retirement record, its |
| - // subscription neither recorded nor settled. |
| - if why, err := s.billingKeepRefused(r.Context(), acct, req.Subscription); err != nil { |
| + // intent's bound session, read now, must be its own, the purchase |
| + // the intent describes, complete and carrying a subscription. Any |
| + // other kept intent could never record and would later expire |
| + // with no retirement record, its subscription neither recorded |
| + // nor settled. The gate is asked only while the occurrence is |
| + // still open: an identical replay of a resolution already made is |
| + // the store's to answer (it repeats without writing), whatever |
| + // has happened to the intent since. |
| + already, err := s.store.billingReview(acct.ID, req.Subscription, req.Reason) |
| + if err != nil && !errors.Is(err, sql.ErrNoRows) { |
| + jsonError(w, http.StatusInternalServerError, "could not read the review. try again") |
| + return |
| + } |
| + if err == nil && already.ResolvedAt != 0 && already.Occurrences == req.Occurrences { |
| + // resolved already: the store decides whether this repeats it |
| + } else if s.billing == nil { |
| + jsonError(w, http.StatusConflict, "billing is not configured on this console, so the intent's session cannot be read; resolve with retire") |
| + return |
| + } else if why, err := s.billingKeepRefused(r.Context(), acct, req.Subscription); err != nil { |
| log.Printf("console: api billing review keep: %v", err) |
| jsonError(w, http.StatusServiceUnavailable, "the intent's session could not be read. try again") |
| return |
| @@ -355,7 +383,7 @@ func (s *Server) handleAPIBillingReviewResolve(w http.ResponseWriter, r *http.Re |
| case errors.Is(err, errBillingReviewStale): |
| jsonError(w, http.StatusConflict, "the review recurred since it was read. read it again") |
| case errors.Is(err, errBillingReviewResolved): |
| - jsonError(w, http.StatusConflict, "that occurrence was already resolved with a different note") |
| + jsonError(w, http.StatusConflict, "that occurrence was already resolved with a different note or outcome") |
| case errors.Is(err, errBillingReviewActor): |
| http.NotFound(w, r) |
| default: |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index 5582ef846..7517c1a50 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -279,19 +279,29 @@ func (b *billingRuntime) ownSession(session *billingstripe.Session, intent billi |
| return intent.SessionID == "" || intent.SessionID == session.ID |
| } |
| +// billingIdentityOfAccount is errBillingIdentity for a refusal that knew |
| +// which account it was settling (a retired intent's event), so the record |
| +// of the refusal can name it. |
| +type billingIdentityOfAccount struct{ AccountID int64 } |
| + |
| +func (e *billingIdentityOfAccount) Error() string { return errBillingIdentity.Error() } |
| +func (e *billingIdentityOfAccount) Unwrap() error { return errBillingIdentity } |
| + |
| // ownBoundSession is ownSession for a read of the session an intent is |
| // bound to, with one more thing when the answer is no: the console created |
| // that session and judged it its own when it bound it, so a read of it |
| // that names another account, customer, intent or mode is a contradiction |
| -// at the provider that a person must see. If the session is still open it |
| -// is expired first, by the id the console itself bound (unique to this |
| -// account and mode at the provider, so the expiry can make nothing but |
| -// this intent's purchase unpayable), and the answer is used for nothing: |
| +// at the provider that a person must see. The intent-keyed review is |
| +// filed, and then, if the session is still open, it is expired by the id |
| +// the console itself bound (unique to this account and mode at the |
| +// provider, so the expiry can make nothing but this intent's purchase |
| +// unpayable), with the answer used for nothing: |
| // what the provider says of a session it has contradicted proves nothing, |
| // so the intent is retired on no such proof. Then the intent-keyed review |
| // is filed with the session as its reference (the same review a |
| // mismatched purchase files); the intent stands, so no second purchase |
| // opens beside it, until the operator resolves the review, which retires |
| +// it or keeps it for a corrected purchase to record |
| // it. A read that is not of the bound session (a listing, a create |
| // response) files nothing: nothing was ever disclosed on it. |
| func (s *Server) ownBoundSession(ctx context.Context, session *billingstripe.Session, intent billingIntent, acct *Account, now time.Time) (bool, error) { |
| @@ -301,17 +311,21 @@ func (s *Server) ownBoundSession(ctx context.Context, session *billingstripe.Ses |
| if session == nil || intent.SessionID == "" || session.ID != intent.SessionID { |
| return false, nil |
| } |
| + // The review first, then the expiry: once the review is filed no |
| + // ordinary retirement can take the intent (retireIntent's open-review |
| + // clause), so a pass that reads the expired session before this call |
| + // returns, or a crash between the two, leaves the intent standing |
| + // under its review rather than retired with no record. An expiry that |
| + // fails leaves the review standing too: the next read files nothing |
| + // new (the review is open) and tries the expiry again. |
| + if err := s.store.fileCheckoutRefusedReview(intent, session.ID, now); err != nil { |
| + return false, err |
| + } |
| if session.Status == "open" { |
| if _, err := s.billing.client.ExpireCheckout(ctx, intent.SessionID, "carloku-expire-"+intent.ID); err != nil { |
| - // Not expired (the provider refused, or the session completed |
| - // under the read): nothing is filed on this pass; the next |
| - // read finds the session as it is now and files then. |
| return false, err |
| } |
| } |
| - if err := s.store.fileCheckoutRefusedReview(intent, session.ID, now); err != nil { |
| - return false, err |
| - } |
| return false, nil |
| } |
| @@ -323,14 +337,16 @@ func (s *Server) ownBoundSession(ctx context.Context, session *billingstripe.Ses |
| // |
| // An open one is expired at the provider, so its URL stops working for a |
| // buyer who still has it, and once the provider says it is expired the |
| -// intent is retired, whether it was bound to the session or not: the next |
| +// intent is retired (unless a review of it is open, which the resolution |
| +// alone ends), whether it was bound to the session or not: the next |
| // attempt starts clean under a new key. |
| // |
| // A completed one cannot be expired: somebody paid for it, in the wrong |
| // currency, which the provider should not have let happen. Nothing is |
| // recorded from it, the intent is left as it is so no second purchase is |
| // opened beside it, a review is filed for the account (once while it is |
| -// open; resolving it retires the intent, so it does not come back), and |
| +// open; resolving it retires the intent, or keeps one whose session can |
| +// still record, after which a session still wrong files it again), and |
| // nothing is written at the provider again (later passes still read the |
| // session, and file nothing more). A person has to look at what was paid |
| // for. |
| @@ -804,7 +820,7 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| if retired, err := s.store.billingIntentRetired(acct.ID, sub.Metadata["checkout_intent"]); err != nil { |
| return err |
| } else if retired { |
| - return errBillingIdentity |
| + return &billingIdentityOfAccount{AccountID: acct.ID} |
| } |
| } |
| refuse := func() error { |
| @@ -826,14 +842,17 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| // still open). The provider tries again. |
| return errBillingPending |
| } |
| + // The review before the expiry, as in ownBoundSession: with the |
| + // review filed, no pass can retire the intent between the |
| + // provider's expiry and the filing. |
| + if err := s.store.fileCheckoutRefusedReview(*pending, pending.SessionID, now); err != nil { |
| + return err |
| + } |
| if session.Status == "open" { |
| if _, err := b.client.ExpireCheckout(ctx, pending.SessionID, "carloku-expire-"+pending.ID); err != nil { |
| return err |
| } |
| } |
| - if err := s.store.fileCheckoutRefusedReview(*pending, pending.SessionID, now); err != nil { |
| - return err |
| - } |
| } |
| return errBillingIdentity |
| } |
| @@ -1115,7 +1134,7 @@ func (s *Server) bindBillingCheckoutFor(ctx context.Context, sessionID string, e |
| if retired, rerr := s.store.billingIntentRetired(acct.ID, session.Metadata["checkout_intent"]); rerr != nil { |
| return "", rerr |
| } else if retired { |
| - return "", errBillingIdentity |
| + return "", &billingIdentityOfAccount{AccountID: acct.ID} |
| } |
| } |
| if err != nil { |
| @@ -1139,7 +1158,8 @@ func (s *Server) bindBillingCheckoutFor(ctx context.Context, sessionID string, e |
| // many passes, webhooks or bindings meet it, and filed only while |
| // that intent stands; the intent stays as it is so no second |
| // purchase opens beside it, until the operator resolves the review, |
| - // which retires exactly that intent. |
| + // which retires exactly that intent or keeps it for a purchase |
| + // corrected at the provider to record. |
| if err := s.store.fileCheckoutRefusedReview(intent, session.ID, time.Now()); err != nil { |
| return "", err |
| } |
diff --git a/internal/console/billingservice_test.go b/internal/console/billingservice_test.go| index ab34a45e3..aec2467b9 100644 |
| --- a/internal/console/billingservice_test.go |
| +++ b/internal/console/billingservice_test.go |
| @@ -63,6 +63,10 @@ type fakeBillingStripe struct { |
| // happens while a reconciliation is paused between its checks and its |
| // record (an operator resolving a review, say). |
| customerHook func() |
| + // expireHook, when set, runs inside the expiry, once, after the fake |
| + // has marked the session expired and before it answers: what happens |
| + // between the provider's expiry and the console's next step. |
| + expireHook func() |
| // sessionAnswers, when set, are applied to the fake in order, one per |
| // rendering of the session, before it is rendered: the provider |
| // answering successive reads of one session differently. |
| @@ -271,6 +275,11 @@ func (f *fakeBillingStripe) serve(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| f.sessionStatus = "expired" |
| + if f.expireHook != nil { |
| + hook := f.expireHook |
| + f.expireHook = nil |
| + hook() |
| + } |
| // The answer names the session that was asked for (as the provider |
| // does), with the metadata the fake was told to answer with. |
| expiredID := strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/v1/checkout/sessions/"), "/expire") |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index d2e040d82..7d1e76433 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -903,8 +903,8 @@ func validReviewText(v string, max int) bool { |
| // and only if it has not recurred since the operator read it. The row update |
| // and the audit append share one transaction. Repeating the exact resolution |
| // of an already resolved occurrence succeeds without writing; a different |
| -// actor or note for it is errBillingReviewResolved. Its one caller is the |
| -// operator door in billingoperator.go. |
| +// actor, note or outcome for it is errBillingReviewResolved. Its one caller |
| +// is the operator door in billingoperator.go. |
| func (s *Store) resolveBillingReview(r billingReviewResolution, now time.Time) error { |
| if r.AccountID <= 0 || r.Subscription == "" || r.Reason == "" || r.Occurrence <= 0 || now.Unix() <= 0 || |
| !validReviewText(r.Actor, billingReviewActorMax) || !validReviewText(r.Note, billingReviewNoteMax) { |
| @@ -1017,7 +1017,8 @@ func (s *Store) resolveBillingReview(r billingReviewResolution, now time.Time) e |
| // fileCheckoutRefusedReview files the review of a session that completed |
| // 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), |
| +// the intent's id, which is what the operator's resolution then retires or |
| +// keeps), |
| // 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 |