amadan
Merge commit 'e152df30' into feat/carloku-billing-operator-cli-20261002
18 files changed,
+1595
−41
docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md+2 −1docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review22.md+243 −0docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review23.md+170 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+43 −2docs/superpowers/specs/OPUS-REVIEW-multi-currency-review22.md+183 −0docs/superpowers/specs/OPUS-REVIEW-multi-currency-review23.md+103 −0internal/billingstripe/catalog.go+30 −6internal/billingstripe/catalog_test.go+50 −0internal/console/billingcurrency_test.go+10 −3internal/console/billingnamed_test.go+504 −0internal/console/billingoperator.go+8 −5internal/console/billingpass.go+6 −1internal/console/billingretirement.go+73 −13internal/console/billingservice.go+17 −5internal/console/billingservice_test.go+9 −0internal/console/billingstore.go+122 −3internal/console/ownership.go+17 −1internal/console/store.go+5 −1
CI passed — run details
diff --git a/docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md b/docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md| index 391d8f1ed..5cedfbf20 100644 |
| --- a/docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md |
| +++ b/docs/superpowers/specs/2026-10-02-intent-retirement-protocol-design.md |
| @@ -123,8 +123,9 @@ Built in `internal/console/billingretirement.go` and the paths it touches, with |
| - **A stale observation after `keep`** (a pass that read a mismatch before the keep and files after it) reopens the review as a new occurrence while the intent still stands: one more hold, visible, never a silent record. A subscription other than the kept session's own that is seen while the kept intent still stands is attached and reopens the review the same way; seen after the purchase recorded and the intent was consumed, it is a dispute for the kept session. A dispute raised between the `keep` and the record stops the record as a conflict. |
| - **The pass's refusal turn** runs under its own deadline whether the reconciliation finished, failed or ran out of time (both turns are children of the caller's context, so cancelling it stops either; production starts the pass, as it starts every console pass, on a context that is never cancelled, so in production a stop is the process ending mid-turn, which the protocol tolerates because each step is one transaction or is redone by the next pass), so slow reconciliation cannot leave a refused session payable pass after pass. |
| - **An unbound intent under a review that names no session** is still reconciled by the pass, which is how its session is found. Nothing records, retires or cleans up an intent under an open review on that path: the store's record, expiry and cleanup each refuse it. (Round twenty's build let the cleanup of an intent that never had a session delete it under its review; review twenty-one.) |
| -- **Named subscriptions** (review twenty-one, answered as one rule). A subscription that names a standing intent is recorded from the first read, whether or not a refusal review exists. Nothing deletes an intent past one. The record of the purchase keeps what it consumed (`billing_consumed_intents`: the intent, its session, the subscription it recorded with) and in the same transaction makes every other subscription seen naming the intent a dispute; a subscription that names the intent afterwards is compared with that record and is a dispute. The expiry of the intent's session, and the cleanup of an intent that never had a session, do not delete an intent a subscription was seen naming: they file the refusal, with it attached, and the intent stands (an expired session has no subscription and an intent with no session never had a checkout, so that subscription was made outside checkout). `retire` is held while anything is attached, whether or not the intent still stands. What stays outside: a subscription first seen naming an intent that already expired in the ordinary way is a rejected event with its account and no hold, because nothing is kept of such an intent. |
| +- **Named subscriptions** (review twenty-one, answered as one rule). A subscription that names a standing intent is recorded from the first read, whether or not a refusal review exists. Nothing deletes an intent past one. The record of the purchase keeps what it consumed (`billing_consumed_intents`: the intent, its session, the subscription it recorded with) and in the same transaction makes every other subscription seen naming the intent a dispute; a subscription that names the intent afterwards is compared with that record and is a dispute. The expiry of the intent's session, and the cleanup of an intent that never had a session, do not delete an intent a subscription was seen naming: they file the refusal, with it attached, and the intent stands (an expired session has no subscription and an intent with no session never had a checkout, so that subscription was made outside checkout). `retire` is held while anything is attached, whether or not the intent still stands. What stays outside: a subscription first seen naming an intent that went with nothing seen naming it and no purchase (one that expired in the ordinary way; one that never had a session and was cleaned up) is a rejected event with its account and no hold, because nothing is kept of such an intent. What the rule costs: a subscription seen naming an intent holds that intent's expiry and cleanup whatever the subscription's own state, a dead one included, into a refusal stage one can only hold; and a second subscription at a purchase's record is a dispute stage one cannot resolve. If the provider ever makes a subscription before its checkout session completes, an honest buyer meets this and is held with no disposition; whether it does is the first check of the test-mode rehearsal, and a gate on enabling (review twenty-two). |
| - **Only a first purchase consumes an intent**: the record of a subscription that is not yet the account's recorded one, judged in the record's transaction. A later event of the account's recorded subscription deletes no intent, whatever its metadata names. |
| +- **A refusal filed late.** A refusal of a session filed for an intent that a purchase consumed meanwhile is kept as a dispute against what was consumed, in the filing's transaction. **Account retirement** refuses while a checkout refusal or dispute is open, and has a point of no return in the database (`commitBillingRetirement`), taken before anything outside is written: checkout evidence filed before it withdraws the pending retirement; evidence filed after it stays with the account, whose deletion finishes, and is listed and resolvable under the deleted account (review twenty-three). **A click** reads the intent's state again after each provider read and before it discloses a new link, and offers nothing under an open review (review twenty-two). |
| - **Binding.** A session id is written onto an intent in one place, which refuses a session that is already another purchase's (another intent's, a retired, kept or consumed one's) and one that is not the session the intent's open review names. |
| - **Creating an intent** re-reads the account's customer and the currency it is held to in its own transaction; a request built from an older reading is refused. |
| - **The refusals' turn** takes the longest-due first. **A listing** that fails on a later page records the pages it read. **An answer carrying another session's id** records nothing. |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review22.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review22.md| new file mode 100644 |
| index 000000000..269a73d10 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review22.md |
| @@ -0,0 +1,243 @@ |
| +All platform references below are at **`05ccec07`**; pricing-site references are at **`01b62f7`**. This was source-only review. Nothing was modified, tested, or accessed over the network. Mutation counterfactuals are judgments from source, not reproduced runs. |
| + |
| +**Twenty-first review, finding by finding** |
| + |
| +| Finding | Closed? | Would its regression test pass without its fix? | |
| +|---|---|---| |
| +| **Opus P2-1 / Astra 1, P2:** cleanup deletes an unready intent under review, then retirement releases its obligations | **Closed.** Cleanup checks the open review (`billingstore.go:604`); the trigger covers that state (`billingretirement.go:205`); `held()` checks protocol and obligations before permitting a gone-intent retirement (`:251`). | **No.** `billingnamed_test.go:56`, `:89`, and `:124` distinguish the three protections. Removing only cleanup’s predicate still fails: the remaining trigger produces an error where the production-path test requires a quiet refusal. | |
| +| **Opus P3-1:** binding moves an intent onto another reviewed or archived session | **Closed for the reported cases.** Both writers use `bindIntentSession`, which checks existing ownership and the review reference (`billingstore.go:433`; `billingretirement.go:687`). | **No.** `billingnamed_test.go:425` covers retired and consumed sessions, the review’s different session, the webhook binder, and repeat binding. | |
| +| **Opus P3-2:** a subscription observed beside an open session disappears on expiry | **Closed.** Its first subscription read is retained; expiry files a refusal instead of deleting (`billingretirement.go:590`, `:662`; `billingstore.go:530`). | **No.** `billingnamed_test.go:195` checks both pass and click, the reference, the initially unproven fate, and held retirement. | |
| +| **Opus P3-3:** an answer carrying another session ID is classified against that other session | **Closed.** `readCheckout` records only a matching returned ID (`billingretirement.go:793`). Consumers still perform the refusal: the wrapper itself returns the mismatched object without an error. | **No.** `billingnamed_test.go:586` requires no dispute for the mismatched request, then a dispute when the same object is requested by its actual ID. | |
| +| **Astra 2, P2:** named subscriptions disappear on ordinary consumption or a late refusal filing | **Closed for the reported subscription evidence.** Classification retains observations without a review; consumption archives the purchase and disputes other observed subscriptions (`billingretirement.go:590`; `billingstore.go:911`). **Session-only refusal evidence still has a related hole: Finding 2 below.** | **No.** The three cases at `billingnamed_test.go:261` distinguish observation before consumption, first observation afterwards, and delayed subscription-refusal filing. | |
| +| **Astra 3, P2:** stale customer and held currency enter a new intent | **Closed.** Both are checked inside `beginBillingCheckout` (`billingstore.go:393`, `:396`). | **No.** `billingnamed_test.go:538` changes each independently and also accepts the current values. | |
| +| **Astra 4, P3:** later listing failure discards earlier pages | **Closed for the reported later-request failure. Not complete for every decoded page:** Finding 4 below. | **No** for the reported failure: `billingnamed_test.go:611` and `billingstripe/catalog_test.go:308` require retained earlier-page evidence. Those tests **pass with Finding 4**. | |
| +| **Astra 5, P3:** older refusals repeatedly exhaust the batch | **Closed for the reported ordering.** Eligible refusals are sorted by due time (`billingretirement.go:933`). | **No.** `billingnamed_test.go:627` supplies five different due times and requires exactly the longest-due three. | |
| +| **Additional defect found while answering:** an already-recorded subscription deletes the newer intent named in its metadata | **Real at `b260ac8e`, and closed.** The old deletion ran whenever `next.IntentID` was nonempty. It now additionally requires a different recorded subscription (`billingstore.go:868`). | **No.** `billingnamed_test.go:366` moves the period to avoid the unchanged-state fast path, then asserts that the newer intent remains and has no consumption archive. | |
| + |
| +The twenty-first reports’ claims have these dispositions: |
| + |
| +| Claim | Assessment | |
| +|---|---| |
| +| **Opus claim 1:** nothing read about a refused purchase is dropped; attribution never follows the answer | **Partly corrected.** Requested-ID attribution and the reported named-subscription losses are fixed. Findings 2 and 4 prevent accepting the universal evidence claim. | |
| +| **Opus claim 2:** nothing retires or cleans up under an unbound review | **Closed for current production deletion paths.** Record, expiry and cleanup now refuse an open review. | |
| +| **Opus claim 3:** every subscription naming an unbound intent files a review | **Still overstated in the governing design, line 116.** An on-terms subscription reaches `billingservice.go:998` and retries without a review. It is now retained as an obligation. | |
| +| **Opus claim 4:** the site formatter-wiring assertion is textual | **Still true, accurately limited.** The separate rendering and formatter tests supply behavioral coverage. | |
| +| **Astra:** the disappeared intent bypasses holds; late subscription filing can become nothing; earlier listing pages disappear | **The reported cases are closed**, with the narrower residuals described below. | |
| +| **Astra:** create answers were excluded in prose but a refused completed create answer was recorded | **Header corrected** at `billingretirement.go:79`. The design’s exclusion at line 115 still omits that exception. | |
| +| **Astra:** structural recording-order enforcement is lexical; mutation runs do not establish every interleaving | **Accurately acknowledged.** Those limitations remain. | |
| + |
| +On severity, **Astra’s P2 reading was right for losing an outstanding subscription across consumption or expiry**: the consequence is an unheld financial obligation, not merely incomplete diagnostics. Opus’s proposed immediate refusal whenever the session reads `open` was too strong: the code cannot establish from that read that an honest subscription is unrelated. Recording it and waiting for the session’s identity is the correct distinction. |
| + |
| +The earlier disputed keep-gate and dispute-resolution cases also warranted P2: one could authorize retirement using obsolete expiry evidence; the other released an invoice hold on a note. Both reported paths remain closed. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P2 — Account retirement ignores the new durable checkout disputes.** |
| + |
| + **Where:** `internal/console/billingstore.go:1479`, `:1486`, `:1506`; `internal/console/ownership.go:151`, `:731`, `:749`; `internal/console/billingoperator.go:203`, `:340`. |
| + |
| + **Scenario:** |
| + |
| + - H is observed naming intent I. |
| + - I’s legitimate subscription C records. The new consumption transaction correctly creates a dispute for H. |
| + - C subsequently ends. There is no standing intent, unfinished month, pending quantity obligation, or quantity-history gap. |
| + - H remains active at the provider and its dispute remains open. |
| + - Account-deletion reservation passes. `checkBillingRetirement` checks none of the checkout reviews, disputes, or attached subscriptions. The console can tombstone and delete the account. |
| + |
| + The review and evidence are **not erased or resolved**. The defect is that account retirement proceeds despite the known unsettled subscription. Normal access to the case also deteriorates: the review listing’s account lookup excludes deleted accounts, and the resolution door’s lookup returns 404. |
| + |
| + **Smallest acceptable fix:** include unresolved checkout-refusal and checkout-dispute cases in `checkBillingRetirement`, inside the transaction shared by reservation, deletion and reaping. Add a consumption → dispute → cancellation of C → attempted account-deletion regression. The new consumption tests stop before this boundary and would pass without that fix. |
| + |
| +2. **P2 — A first session refusal can disappear when another console consumes the intent.** |
| + |
| + **Where:** `internal/console/billingservice.go:1267`, `:1278`; `internal/console/billingstore.go:1239`; `internal/console/billingretirement.go:393`, `:582`, `:838`. |
| + |
| + **Scenario:** |
| + |
| + - Bound session S belongs to ordinary intent I; no refusal review exists. |
| + - Console A reads S completed with purchase metadata that fails `intentSession`, such as a different plan. Its initial recording wrapper retains nothing because no review exists. |
| + - A reads I and pauses immediately before filing the refusal. |
| + - The metadata is corrected. Console B reads the valid session and subscription C, records the purchase, and consumes I. |
| + - A resumes `fileRefusal(I, S, "", session, now)`. |
| + - The gone-intent branch calls `classifySubscriptionTx` with an empty subscription, which does nothing. |
| + - The subsequent `noteSessionSeen` cannot find the ordinary consumed purchase: `purchaseOfSessionTx` searches standing, referenced, retired and kept purchases, **not `billing_consumed_intents`**. |
| + |
| + A’s refusal is acknowledged through the rejected-event path, but no review or dispute preserves the refused completion. This is the refusal-producing read that the implementation expressly promises to hand in again, not an unrelated ordinary late read. |
| + |
| + **Smallest acceptable fix:** preserve a session-only refusal against the consumed purchase’s archive, with its recorded account/session identity, before acknowledging it. Merely adding an archive lookup is insufficient if a purchase-field mismatch still produces no dispute: preserve the actual refusal as well. Add the two-store ordering above. The new delayed-filing test supplies `"sub_hand"` and therefore misses this case. |
| + |
| +3. **P2 — A checkout click can return a payable URL after freshly reading an open refusal review.** |
| + |
| + **Where:** `internal/console/billingservice.go:614`, `:651`, `:713`, `:728`; `internal/console/billingstore.go:407`. |
| + |
| + **Scenario:** |
| + |
| + - A click passes the initial open-review check. |
| + - While either checkout GET is in flight, another console files a refusal for I. Its expiry attempt fails, leaving S open. |
| + - The click receives an otherwise valid open session. |
| + - Its fresh `refusalState` read sees `Open=true`, but both post-read checks inspect only `Disputed`. |
| + - `beginBillingCheckout` can return the existing intent without checking the review, and the click returns its URL. |
| + |
| + The intent remains held; this does not silently record the purchase. It nevertheless actively offers payment through a checkout the console already knows cannot record without operator action. |
| + |
| + **Smallest acceptable fix:** stop on `st.Open || st.Disputed != 0` after both reads, and apply the same check before disclosing recovered or newly saved checkout URLs. Add first-read and second-read hooks that file an open review **without a dispute**, leave expiry unsuccessful, and require no URL. Existing dispute-resume tests would pass without this fix. |
| + |
| +4. **P3 — A broken listing page still discards decoded evidence at and after its duplicate entry.** |
| + |
| + **Where:** `internal/billingstripe/catalog.go:164`; `internal/console/billingretirement.go:806`. |
| + |
| + **Scenario:** |
| + |
| + - An earlier page contains archived session S as expired. |
| + - A later successfully decoded page contains S again, now complete, followed by another relevant session. |
| + - The duplicate-ID check returns `out` **before appending the duplicate or examining the remaining decoded entries**. |
| + - The recording wrapper sees only the earlier expiry. The contradictory completion it actually received is lost. |
| + |
| + Rejecting the pagination is correct. Discarding its decoded evidence is inconsistent with the new listing contract. |
| + |
| + **Smallest acceptable fix:** separate evidence delivery from pagination acceptance. Return or record all attributable decoded session observations, including duplicate-ID observations, while still returning the pagination error and offering no recovery result. Add a contradictory duplicate-page test and a valid relevant entry after the duplicate. The new later-HTTP-failure tests do not distinguish this case. |
| + |
| +**Claims the code contradicts and the remaining requested checks** |
| + |
| +The broad lifecycle claim is still too strong because of Findings 1–3. The original failed-expiry deletion sequence is closed, but that does not establish the whole retirement/evidence family as closed. |
| + |
| +Other specific prose corrections: |
| + |
| +- `billingservice.go:778` still describes unready deletion preparation as unconditional. It now respects named subscriptions and open reviews. |
| +- `billingretirement.go:911` says a retried refusal becomes due “an hour later at earliest.” The schedule at `:745` is one, five, fifteen minutes, then hourly. |
| +- Retirement-design line 116 still says an unbound named subscription files a refusal without qualifying the on-terms retry. |
| +- Retirement-design line 115 still excludes create answers without the completed-refused-create exception now documented in the implementation. |
| +- `readCheckout`’s “failed read” wording means consumers refuse a wrong-ID answer; the wrapper itself returns that object successfully. Its new test explicitly requires this behavior. |
| + |
| +The money path remains consistent in the inspected code. For **GET `?country=IN¤cy=gbp`**, the page selects India’s band D independently of GBP; Personal is £7.70 annually. The form carries the signed country, band, effective currency, choice flag and member count. The handler compares those with what the posted fields would buy before calling checkout. The intent pins GBP and the book; Checkout receives GBP with Adaptive Pricing disabled; binding and reconciliation require the same price terms and currency. The assignment, monthly allowance, version-2 calculation, invoice requests, invoice recovery and quantity worker retain GBP. I found no path in that trace that converts or silently substitutes EUR. |
| + |
| +| Requested area | Source conclusion | |
| +|---|---| |
| +| **One form, crafted posts, stale tabs** | `billinghandlers.go:71` compares the effective purchase choice with a server-authenticated quote. An old tab can buy its own still-valid quote; changing cookies in another tab does not change its posted purchase. Changed or fabricated quote fields require another quote. | |
| +| **Present, absent, empty and repeated currency fields** | `billingFormChoice` (`billinghandlers.go:176`) refuses present-empty, repeated, uppercase and unsupported values. An absent field supplies no explicit choice; against a book console the effective country default still must match a signed quote. Legacy checkout retains its legacy behavior. | |
| +| **Quote signature** | The key derives from the server’s billing secret, not the disclosed CSRF token. The signature covers session token, account, book, country, band, currency, choice flag and member count. Copying disclosed fields does not permit a different quote. Another session/account/book/token invalidates it. | |
| +| **Unsigned plan, quantity and postal fields** | The page quotes all offered plans. Charged quantity comes from membership and is checked again under the membership write lock. Postal information affects tax/location validation rather than the signed base price. Membership changes cause a conflict instead of silently changing the quote’s quantity. | |
| +| **Quote door** | Session, form token and membership precede a redirect constructed to the account’s billing page (`billinghandlers.go:137`). Posted country/currency cannot select an external destination, and this door creates no checkout. | |
| +| **Cookies** | Present-empty clears and suppresses fallback; absent falls back to a validated cookie; invalid sets nothing. Therefore an invalid parameter does not erase an old cookie, which can reappear on a later absent parameter. Checkout itself uses the form, not the cookie. | |
| +| **Held currency, including legacy purchases** | The page offers only the held currency. Legacy historical price terms mean EUR. Service and transactional intent creation enforce the held currency even after cancellation. | |
| +| **Allowance span trigger** | `billingstore.go:208` uses positive overlap ending at the earliest of the next assignment, paid-through `period_end`, and month end. It searches both assignment tables and handles same-second supersession by revision. This agrees with `AllowanceForMonth`; I found neither requested valid-history counterexample. | |
| +| **Assignment writers** | Renewal and quantity changes append effective revisions; `period_end` remains the provider’s paid-through limit, not the next revision’s start. Inactive and free assignments do not supply paid book spans. | |
| +| **Trigger migration** | The new span trigger exists before `retireEarlierBarrierTrigger` verifies it and drops the old name transactionally. Missing new trigger leaves the old one intact; a second start is idempotent. A prohibited older binary can recreate the old name, so the rollout prohibition remains material. | |
| +| **Inclusions** | The console divides the pooled allowance by the charged quantity. Unavailable Personal is calculated at its own valid quantity. Current book/terms values make the displayed shares exact, including teams above minimum. Band-A sentences are checked verbatim. `formatFigure` itself is not a general exactness guarantee for future fractional data. | |
| +| **Book and catalogs** | Validation checks completeness, duplicate keys, supported country/currency shapes, rounding and overflow, and legacy-EUR agreement. Configuration requires the current catalog and accepts whole optional catalogs. Provisioning’s own-only `currency_options` rule agrees with the console. Its source test does not execute the shell verifier. | |
| +| **Snapshots and invoices** | Stored book/currency identity, mixed-money monthly refusal, calculation currency, invoice/item/final-line checks and recovery checks remain intact. Historical legacy bytes and request shapes are explicitly preserved. | |
| +| **Public export** | It contains public commercial terms and country mappings, not provider credentials or catalog IDs. I found no new disclosure or formatting defect. | |
| + |
| +For **section K**, the supplied site export matches the platform export. Its renderer derives the monetary slots, exact overage rates and floors from that export; inclusion scaling follows the band factors and rejects inexact shares. Country options and their default currencies come from the book. Static and scripted signup links retain both parameters, sending currency only after an explicit selection and country as selected. |
| + |
| +The draft marker and the exact `PRICEBOOK_ALLOW_DRAFT === "1"` checks agree between renderer and checker. Approved numeric literals use whole-token checks; rendered slots are compared with the export in every currency. The executable `moneyText` test rejects fractional minor units. The remaining textual formatter-wiring assertion is supplementary, not the sole coverage. I found no new section-K defect. |
| + |
| +The ownership predicate has the requested account metadata, client reference, mode, intent, known customer and bound-session clauses (`billingservice.go:273`). Purchase validation follows ownership and adds plan, band, terms, book and fixed-currency checks. There are two deliberate exceptions to “everything passes `ownSession` before any action”: |
| + |
| +- Recording wrappers classify evidence before the service’s acceptance decision. |
| +- `advanceRefusal` can expire the console’s own bound ID even when its returned metadata contradicts ownership. That is authority derived from the existing binding, not from the contradictory answer. |
| + |
| +Expiring that trusted bound ID is the right choice under the stated ID-uniqueness assumption. A mismatched create/listing response that was never bound does not grant that authority. A referenced-but-unbound session may remain open until provider expiry; stage one does not expire it by reference alone. |
| + |
| +Review-before-expiry now protects the original race: expiry failure or process death leaves the review standing; payment winning expiry records completion rather than authorizing retirement. A review can intentionally describe a session whose expiry failed. Reconcile’s read/expiry failures remain retryable webhook 503s; completed identity refusals settle through the rejected-event path. Store-recording failure is not acknowledged as successful processing. |
| + |
| +The provider-read inventory is structurally centralized: |
| + |
| +| Path | Recording method | |
| +|---|---| |
| +| Initial and locked subscription reconciliation | `readSubscription` | |
| +| Reconcile’s bound-session and refusal reads | `readCheckout` | |
| +| Binder, including archived and recorded-subscription paths | `readCheckout` | |
| +| Both checkout resume reads | `readCheckout` | |
| +| Pending-intent pass | `readCheckout` | |
| +| Recovery listing | `listCheckouts` | |
| +| Operator keep gate | `readCheckout` | |
| +| Both refusal-advancement GETs | `readCheckout` | |
| +| Quantity worker and invoice preflight | `readSubscription` | |
| + |
| +The only direct calls to the three provider reads in production console source are their wrappers. Provider errors return before classification; wrong requested ID, wrong subscription mode/ID, and unidentifiable metadata deliberately produce no classification. Store failures return no usable object. Finding 4 concerns evidence discarded **inside the listing client**, before the wrapper can receive it. |
| + |
| +The structural test detects receiver aliases, method values, method expressions, interfaces and same-named methods on another type. Exported provider methods must be classified. It does not prove control flow: an unreachable recording call can satisfy its lexical ordering check. A differently named read on an unrelated client type is also outside that inventory. |
| + |
| +There are exactly **four production intent-delete statements**: |
| + |
| +| Statement | Entry paths and result for a previously recorded named subscription | |
| +|---|---| |
| +| `billingstore.go:553`, ordinary expiry | Pass, changed/expired checkout click, and `refuseSession` through bound or unbound expiry. A recorded named subscription causes a refusal and preserves I. An existing open review prevents deletion. | |
| +| `billingstore.go:604`, unready cleanup | Pass after 25 hours, replacement click, account-deletion preparation. A recorded named subscription files the refusal; an open review prevents deletion. | |
| +| `billingstore.go:890`, first-purchase consumption | Production reconciliation. Exact intent consumption, account/assignment/event/queue changes, archive and disputes commit together. Other recorded subscriptions become disputes; C itself does not. | |
| +| `billingstore.go:1203`, operator retirement | The resolution transaction checks protocol, fate, dispute and obligations, archives exactly the reviewed intent, closes its pending checkout quantity events and deletes it. | |
| + |
| +The two session-ID UPDATE branches are both inside `bindIntentSession` (`billingstore.go:458`, `:461`). Same-session rebinding and recovery remain allowed. An open review with no session reference can acquire its recovered session; one naming a different session refuses it. Archived session reuse is refused. The code assumes provider session IDs are not legitimately recycled. |
| + |
| +There is **no account-deletion cascade** on intents: accounts are soft-deleted. Finding 1 is a missing account-retirement guard, not an extra intent DELETE. |
| + |
| +For observations already committed to the store, SQLite transaction ordering closes the named-subscription deletion races: classification first blocks expiry/cleanup or supplies consumption’s dispute; consumption first supplies the archive for later classification. Retirement first supplies its retirement archive. |
| + |
| +The stated no-archive exception is broader in practice than “ordinary expiry” alone. Cleanup of an unready intent also leaves no archive. A subscription first classified after either deletion can become only a rejected event. A provider GET that returns before deletion but whose recording transaction runs afterwards reaches that same boundary. The implementation’s guarantee begins at durable classification, not at the instant network data arrives. |
| + |
| +`billing_consumed_intents` is written only when the transaction actually deletes one intent. Normal renewal and cancellation do not overwrite it. The `INSERT OR REPLACE` is not exercised as replacement by ordinary generated intent IDs; deliberate ID reuse would overwrite historical identity and is not a supported lifecycle. |
| + |
| +Classification checks standing intent, known attachments, retirement, kept resolution, then ordinary consumption. The account’s currently recorded subscription is excluded earlier. Consequently, honest renewal/cancellation and an older purchase’s own later event do not become disputes merely because a newer purchase exists. An empty consumed session ID uses the intent ID for disputes created **during** consumption, but late classification currently returns without a dispute for that archive. Production first-purchase reconciliation requires a bound session, so I found no production route to that empty-session archive; the looser store test helpers can construct one. |
| + |
| +The new customer comparison does not permanently refuse an ordinary retry: the start path passes the account’s current customer, and a never-purchased account legitimately has none. A created-but-unrecorded customer remains on its intent and is recovered there. House status has its separate transactional coupon comparison. |
| + |
| +For an honest buyer, the conservative rule has these consequences: |
| + |
| +| Provider sequence | Console result | |
| +|---|---| |
| +| C arrives before S completes, or before completed S names its subscription | C is retained; reconciliation retries. Once S names C and payment is acceptable, C records without a dispute. | |
| +| Duplicate delivery of the same subscription/event | Attachment is idempotent; an already-seen event is acknowledged. It does not create a second subscription obligation. | |
| +| Failed payment later succeeds using the same C and S | Pending while incomplete/unpaid; ordinary recording once all checks pass. | |
| +| Abandoned authentication or failed payment leaves a named subscription, then S expires | Expiry preserves I and files a refusal. Even `incomplete_expired` does not remove the obligation. The buyer remains held. | |
| +| Delayed payment: S completes before payment settles | Completion is not treated as payment. Recording and keep wait for acceptable payment status. | |
| +| Provider replaces C1 with C2 for the same intent | Both IDs remain observed. Recording C2 disputes C1; stage one cannot distinguish a legitimate replacement from an extra manually created subscription. | |
| +| Cancellation or renewal after C recorded | No fresh intent consumption. Ordinary events retain their original purchase identity. | |
| + |
| +The code can distinguish identical IDs and the subscription S eventually names. It cannot infer that a different subscription was harmless, or that cancellation settled every financial obligation. Whether Stripe produces the problematic honest sequences is an enablement verification requirement. |
| + |
| +Stage-one resolution otherwise retains its conservative properties: |
| + |
| +- Fate is written once; incompatible later terminal/identity evidence creates a dispute. |
| +- Protocol-zero reviews remain held. |
| +- Retirement refuses attached subscriptions, including when the intent is gone. |
| +- Keep accepts only the bound, owned, matching, completed and acceptably paid session; it does **not** validate the subscription’s full purchase terms. A still-wrong subscription can reopen the review on reconciliation, as the governing design now states. |
| +- Exact resolution replay compares actor, note and outcome before requiring fresh provider work. Omitted outcome still means `retire`; historical rows are not reinterpreted as proof of expiry. |
| +- The resolution history is the transactional audit. General audit logging can repeat on an HTTP replay. |
| +- Disputes have no stage-one resolution. The API returns 409; it does not acknowledge them away. |
| +- The refusal trigger is dropped and recreated in one transaction. A failed migration retains the previous committed trigger. Older binaries still do not implement the complete protocol, so the stop-and-no-rollback rule remains necessary. |
| +- `quantityOutcomesSQL` supplies the guard, startup sweep and obligation check. The healing sweep is gone; pre-history and clock-rollback gaps remain held. |
| + |
| +The rejected-event API remains operator-only, newest first, capped at 200, and drains its rows before account lookups. It exposes account/object identifiers to operators, not ordinary members. It is not a universal refusal ledger: pending/503 events, some interactive failures, and provider data outside the recorded identity paths need not appear there. |
| + |
| +The refusal budget is three attempts with a separate 45-second turn, ordered longest-due first. Its context is a child of the caller. This fixes the reported unfair ordering and cancellation defect. It does not promise a small fixed number of passes for an arbitrarily large backlog. Pending-intent work still precedes subscription reconciliation within the separate reconciliation turn. |
| + |
| +The tests establish their named sequential or hooked histories, not every concurrent execution: |
| + |
| +- The new `billingnamed_test.go` cases distinguish the reported fixes as tabled above. |
| +- The first-read, consumption, expiry, cleanup, binding and due-order tests exercise production helpers; customer/currency concurrency is represented by ordered store operations. |
| +- The four previously surviving mutations now have distinguishing cases: separate identity edits, incomplete H reopening before reconciliation can refuse it, a refusal recording fate from its sole read, and contradiction only on the second resume read. |
| +- The attachment-aborting trigger at `billingretirement_test.go:688` tests rollback inside the production refusal helper. |
| +- The expiry hook tests a controlled boundary; it is a stand-in for another actor, not two running consoles. |
| +- The legacy-retirement helper recreates the material pre-protocol state: resolved outcome, absent fate evidence, archived intent, and no barrier during deletion. It tests compatibility, not permission for current code to perform that retirement. |
| +- The new fake’s distinct second session and the resolution replay assertions now distinguish recording the retired completion. |
| +- The changed fixtures are justified: session-ID reuse was unrealistic; customer snapshots must match the account; and a dispute-specific test must remove the newly effective named-subscription hold to reach its intended branch. |
| +- I found no new shared-cookie-jar or temporary-root dependency in these added tests. The schedule tests use fixture time; fairness’s account-creation wall clock does not determine its ordering assertion. |
| +- Review-three/four mutation claims remain appropriately qualified: shell-source checks are textual, successful migration does not prove crash ordering, and deleting by account alone can be redundant behind the same transaction’s exact-intent lookup. Removing an exactness assertion over exact current data still passes; a data mutation is the distinguishing test. |
| + |
| +The source supports the named mutation counterfactuals, but does not independently certify the claimed **27** and **43** executed mutation runs or their exact transformations. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +1. **Catalog identity:** exercise all 36 prices, expanded own-only `currency_options`, exact integer amounts, tax behavior and metadata. The console and provisioning verifier must accept the same legitimate shapes and refuse changed currency/amount/options. |
| +2. **GBP purchase from the one form:** India plus explicit GBP, default-only country choices, and held legacy EUR. Confirm the displayed base amount, actual checkout currency, disabled Adaptive Pricing, subscription/item currencies and stored book identity. |
| +3. **Payment ordering:** deliver subscription events before completion, completion before subscription attachment, duplicates, failed/retried authentication and delayed payments. Establish whether Stripe ever creates, expires or replaces multiple subscription IDs for one checkout; observe the resulting conservative holds. |
| +4. **Expiry versus payment:** retain the checkout URL while expiry succeeds, fails, times out or races payment. A proven expiry must prevent payment; a winning payment must appear as completion and remain held unless kept. |
| +5. **Proof and idempotency:** verify identity fields on GET, listing, create and expiry responses; concurrent identical keys, lost responses, remembered successes/errors and key-retention expiry. A subsequent GET must establish the outcome used for retirement. |
| +6. **Lost-create recovery:** recover through original-key replay and paginated customer listing, including partial failures. Verify session-ID uniqueness and that GET never returns another requested object. |
| +7. **Metadata changes:** exercise permitted account/intent/customer/purchase metadata changes and corrections. Confirm which fields are mutable and how delayed reads behave; contradictions must remain attributable to the recorded purchase. |
| +8. **Financial obligations:** test what cancellation and `incomplete_expired` leave outstanding—payments, collectible invoices, pending collection and refunds. None may be treated as settlement merely from subscription status. |
| +9. **Overages and quantity:** produce a GBP monthly invoice through creation, lost-response recovery, item verification, finalization and payment; perform quantity changes/proration and verify returned price/currency and allowance dating. |
| +10. **Legacy and customer reuse:** replay legacy EUR request shapes unchanged, renew and cancel them, then purchase again using the held customer/currency. Confirm historical invoices remain payable without conversion or repricing. |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review23.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review23.md| new file mode 100644 |
| index 000000000..2092779b0 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review23.md |
| @@ -0,0 +1,170 @@ |
| +All platform references below are at **`2618403f`**; pricing-site references are at **`01b62f7`**. This was source-only review against `12e70d95`, including the delta from `05ccec07`. Nothing was modified or tested, and no network commands were run. Mutation counterfactuals below are judgments from source. |
| + |
| +**Twenty-second review, finding by finding** |
| + |
| +| Finding | Closed? | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **Opus P2-1:** a dead attached subscription can leave an honest buyer held indefinitely | **Addressed honestly as a conservative stage-one limitation and enablement gate, not eliminated.** `billingretirement.go:97` and dispositions `:484`, `:500` explicitly say there is no stage-one disposition. | **Yes: behavior is unchanged.** There is no new behavioral regression proving a status-based release, nor should one be claimed. | |
| +| **Opus P3-1:** unready cleanup is another no-archive exception | **Closed as a documentation finding.** Both ordinary expiry and unready cleanup are named at `billingretirement.go:91` and retirement design `:126`. | Not applicable: the accepted response documents the exception. | |
| +| **Opus P3-2:** subscription attribution follows incorrect or missing account metadata | **Closed.** `accountOfIntentTx`, `billingretirement.go:765`, finds the console’s recorded owner before classification. | **No.** `billingnamed_test.go:888` requires attachment for both missing and incorrect account metadata. | |
| +| **Opus P3-3:** the pass reports a hold it caused as a failure | **Closed at both sites**, `billingpass.go:267` and `:297`. | **No with the whole fix removed.** The named test at `billingnamed_test.go:912` exercises the bound-session branch. Removing only the recovery filter at `billingpass.go:297` is not distinguished by that test. | |
| +| **Opus P3-4:** inconsistent dispute key for consumption without a session | **Closed.** `billingretirement.go:660` uses the intent ID, matching consumption. | **No.** `billingnamed_test.go:938` distinguishes it. This remains a defensive store case: production reconciliation requires a bound session. | |
| +| **Opus claim 1:** only ordinary expiry lacks an archive | **Corrected**, as above. | Not applicable. | |
| +| **Opus claim 2:** the stranded buyer “waits on a person” | **Corrected:** the buyer is held with no available stage-one disposition. | Not applicable. | |
| +| **Opus claim 3:** the ended-subscription regression establishes ordinary provider behavior | **Corrected.** Dispositions `:473` calls it defensive and identifies the fake’s period change. | The regression fails without the consumption fix, but does not establish that Stripe produces its setup. | |
| +| **Astra 1, P2:** retirement ignores open checkout refusals/disputes | **Closed for an already-open case at reservation. The complete retirement boundary remains incomplete:** Finding 1 below. `billingstore.go:1514` adds the transactional check. | **No.** `billingnamed_test.go:711` fails without it. It passes with the remaining post-reservation race. | |
| +| **Astra 2, P2:** a session-only refusal filed after consumption disappears | **Closed.** `billingstore.go:1256` preserves the refusal against the consumed purchase. | **No.** `billingnamed_test.go:758` requires the dispute and open review. It directly exercises delayed filing, rather than orchestrating two consoles. | |
| +| **Astra 3, P2:** a click discloses a link despite freshly observed review | **Closed for the reported cases.** Both post-read checks and the newly saved link check include `Open`: `billingservice.go:653`, `:715`, `:781`. | **No.** The three cases at `billingnamed_test.go:791` distinguish the checks independently. The first changes country and asserts zero expiries, avoiding the previous masking by the second check. | |
| +| **Astra 4, P3:** duplicate-page failure discards decoded observations | **Closed for duplicate pagination. A decoding-error variant remains:** Finding 2 below. | **No for the reported duplicate case.** `billingnamed_test.go:868` and `billingstripe/catalog_test.go:333` distinguish that fix; both pass with Finding 2. | |
| + |
| +For **Opus P2-1**, the disposition is sufficient for conservative **stage one**, provided it remains an enablement gate. Reading an attached subscription’s status is unnecessary merely to continue holding it. Automatically exempting `incomplete_expired` would introduce a settlement policy that stage one deliberately withholds. Astra’s test-mode item 8 means exactly that status alone cannot establish the disposition of payments, invoices, pending collection or refunds. If rehearsal demonstrates the problematic honest-buyer sequence, enabling requires the owner’s settlement decision and an implementation checking its evidence—not just a status exception. |
| + |
| +On severity, Astra’s P2 reading was right for losing outstanding obligations, retiring their account, or offering payment after observing an open refusal. Opus’s conclusion that the deletion/consumption checks closed the whole lifecycle family was too broad. The pass’s one-time diagnostic failure and the unreachable empty-session archive case are appropriately P3. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P2 — A dispute recorded after retirement reservation does not stop the authoritative account tombstone.** |
| + |
| + **Where:** `internal/console/billingstore.go:1527`, `:1539`; `internal/console/ownership.go:151`, `:172`, `:194`; `internal/console/billingretirement.go:653`, `:737`. |
| + |
| + **Concrete scenario:** |
| + |
| + - Legitimate subscription C records, consuming intent I. C later ends. |
| + - Account deletion passes `reserveBillingRetirement`; its transaction commits. |
| + - Before the bucket write, another console reads subscription H naming archived I. Classification **commits** a checkout dispute. |
| + - The deletion request still calls `tombstoneAccount`. |
| + - Only afterwards does `DeleteAccount` rerun `checkBillingRetirement` and refuse. |
| + |
| + This is not the acknowledged boundary between receiving provider data and recording it: the dispute has already committed before external retirement. |
| + |
| + The external effect is real. `internal/domains/mint.go:248` clears the manifest’s owner and sets `DeletedAt`; `:254` publishes it. `internal/domains/reconcile.go:601` subsequently removes the registry account. |
| + |
| + The local account and evidence remain because `DeleteAccount` refuses. However, the authoritative manifest says the account is retired. The reservation remains, retries now fail the review check, and new checkout is blocked by `billingstore.go:363`. Stage one cannot resolve the dispute that prevents completion. |
| + |
| + **Smallest acceptable fix:** coordinate authoritative retirement authorization with evidence recording across consoles. A pending retirement must be cancellable by evidence committed before finalization, or the final check and publication must have a shared serialization boundary. Another standalone preflight check merely narrows the race. Preserve evidence and operator access when retirement has already progressed. |
| + |
| + Add a regression that pauses **after successful reservation and before tombstoning**, records H through the production classification path, then resumes deletion. The current test at `billingnamed_test.go:711` checks only the opposite ordering and passes with this defect. |
| + |
| +2. **P3 — A malformed session discards valid sessions already decoded from the same listing page.** |
| + |
| + **Where:** `internal/billingstripe/catalog.go:152`, `:161`; `internal/billingstripe/client.go:240`, `:351`; recording wrapper at `internal/console/billingretirement.go:858`. |
| + |
| + **Concrete scenario:** |
| + |
| + - A session S was retired on proven expiry. |
| + - A listing response is valid JSON containing a fully valid S, now `complete`, followed by `{"id":"cs_bad"}` without `livemode`. |
| + - JSON decoding successfully populates S before `Session.UnmarshalJSON` rejects the second object. |
| + - `c.do` returns `CodeDecodeError`. |
| + - `listCheckouts` returns only `out`, containing previous pages, and discards the successfully decoded S from this page. |
| + - The recording wrapper never receives the contradictory completion, so no dispute is filed. |
| + |
| + The new loop correctly preserves duplicates **after successful page decoding**. It does not handle this earlier return. |
| + |
| + **Smallest acceptable fix:** decode the page envelope into raw session elements, validate elements independently, and return fully validated observations alongside the page’s error. Preserve valid elements before and after a malformed element; never classify a partially decoded object. Recovery must still fail. |
| + |
| + Add client and wrapper cases containing a contradictory archived session and a malformed neighboring element. The new duplicate-page tests pass without this additional protection. |
| + |
| +**Claims the code contradicts, and the remaining requested checks** |
| + |
| +The broad account-retirement and listing-evidence claims still need qualification because of Findings 1 and 2. Other concrete prose discrepancies remain: |
| + |
| +- `billingstore.go:1217` says a late gone-intent refusal files nothing; the new branch at `:1256` deliberately files a dispute after consumption. The binder’s “only while that intent stands” comment at `billingservice.go:1284` needs the same qualification. |
| +- `billingnamed_test.go:763` describes a session read before consumption. The test calls `purchased` first and reads the session at `:766` afterwards. It establishes delayed-filing handling, not that complete read/record interleaving. |
| +- `billingservice.go:790` calls unready deletion preparation unconditional. The production cleanup respects reviews and recorded named subscriptions. |
| +- `billingretirement.go:965` says retries are at least an hour apart and reach every case within “a few passes however many.” The schedule at `:797` begins at one, five and fifteen minutes; the three-attempt budget cannot promise a fixed small number of passes for an arbitrary backlog. |
| +- Retirement design, “Stage one as built,” `:116` still broadly says a subscription naming an unbound intent files a review. An on-terms one retries at `billingservice.go:1009`, with its obligation retained. |
| +- That design’s create-answer exclusion at `:115` still omits the completed-refused-create exception documented at `billingretirement.go:82`. Its subscription-attribution description also needs to reflect the recorded intent owner taking precedence over account metadata. |
| + |
| +The following conclusions cover the requested pricing, ownership, migration and lifecycle checks. |
| + |
| +| Area | Source conclusion | |
| +|---|---| |
| +| **A: GBP purchase end to end** | India selects band D independently of GBP. Personal renders £7.70 annually. The handler compares the effective posted choice with the authenticated quote (`billinghandlers.go:71`); checkout pins GBP/book (`billingservice.go:603`) and sends fixed-currency parameters (`:221`). Account and assignment storage preserve them (`billingstore.go:681`, `:848`). Monthly allowance rejects mixed price terms (`plans/month.go:167`); calculation produces GBP version 2 (`billingcalculation.go:174`). Invoice creation, recovery and verification retain GBP (`billinginvoice.go:516`, `:569`, `:901`, `:964`, `:1004`, `:1116`), as does the quantity worker (`billingquantity.go:1017`, `:1105`). I found no conversion or silent EUR substitution in this trace. | |
| +| **B, M: one form and effective choice** | Present currency must be exactly one supported value. Empty, repeated, uppercase and unsupported fields fail (`billinghandlers.go:176`). Absent currency supplies no explicit choice; against a book console the country default must still match the signed quote. Changing country/currency, erasing the choice flag or fabricating quote fields cannot authorize an unseen quote. A stale tab can submit its own still-valid quote; another tab’s cookies do not change its posted purchase. | |
| +| **Quote door** | Session, form token and membership precede a redirect constructed to the account’s billing page. Posted values are URL-encoded (`billinghandlers.go:165`, `:197`). This door creates no checkout and cannot choose an external redirect destination. | |
| +| **Q, U, Z: signature** | The key derives from the server’s billing secret; the disclosed CSRF token is message material, not the key (`billingpage.go:442`). The signature covers token, account, book, country, band, currency, choice flag and member count. Another session/account/book/token invalidates it. The page quotes all offered plans; charged quantity derives from membership and plan minimum. Postal information affects location/tax rather than the quoted pre-tax book price. Membership is checked again before charging and transactionally when the intent is created. | |
| +| **C, O: cookies and held currency** | Present-empty clears and suppresses fallback; absent falls back; invalid changes no cookie (`billingcurrency.go:43`). Consequently, an invalid parameter can leave an old cookie available on a later parameter-absent visit. That is the documented rule, not an implicit checkout choice. Held currency is offered alone; legacy purchases hold EUR. Service and transactional intent creation enforce it even after cancellation. | |
| +| **E, N: assignment-span trigger** | `billingstore.go:208` uses positive overlap ending at the earliest of the next assignment, paid-through limit and month end, matching `AllowanceForMonth` (`plans/month.go:141`). It considers both assignment tables and same-second revision ordering. Same-second supersession contributes zero span; inactive/free rows contribute no paid book span. Renewal and quantity writers preserve provider `period_end` as the paid-through limit, distinct from the revision’s effective date (`billingstore.go:845`). I found neither requested valid-history counterexample. | |
| +| **H, T: migration and rollback** | The refined trigger exists before `retireEarlierBarrierTrigger` verifies it and drops the old name transactionally (`billingstore.go:1002`). Missing new trigger leaves the old one; repeated startup is idempotent. An older prohibited build can recreate the old trigger name. The rollout prohibition remains necessary: these barriers do not make an old binary understand book history or the complete retirement protocol. | |
| +| **F: inclusions** | The page divides `PriceFor`’s pooled allowance by charged quantity. Unavailable Personal uses a valid Personal quantity for its disabled row. Current terms yield exact displayed shares, including teams above minimum; the tests check all factors and band-A wording (`billingcurrency_test.go:2554`). Removing an exactness assertion over already-exact data need not fail; changing the data to violate it does. | |
| +| **G, J: book, catalogs and export** | Validation checks strict shape, complete dimensions, duplicate keys, rounding, overflow and legacy-EUR agreement (`pricebook.go:149`). Configuration accepts the required catalog plus whole optional catalogs, with unique provider IDs (`cmd/console/billing.go:106`). Provisioning and console agree on own-only expanded `currency_options`. The export contains public commercial/resource/country data, not credentials or provider catalog identities (`pricebook.go:727`). | |
| +| **K: pricing site** | At `01b62f7`, rates retain exact micro amounts; inclusions use exported factors and exactness checks (`copy-review/pricing/build.mjs:47`, `:113`, `:156`). Draft rendering/checking uses the first-line marker and exact `"1"` override. Country choices/defaults come from the book. Links always carry both parameters, with currency only when chosen (`src/site.js:40`, `:75`, `:89`). A locale-only suggestion may differ from the console’s later country default because the suggestion is intentionally not transmitted as a choice. | |
| +| **MM, QQ: site tests** | Slot values are checked across plans, bands and currencies; rates are parsed back to exported micro amounts. Whole-token checks cover approved numeric literals. A hardcoded value exactly equal to the expected current value still passes an output-equality test; the test explicitly acknowledges that limitation (`pricing-pricebook.test.mjs:202`). `hack/money.mjs` supplies the production formatter exercised by the fractional-minor test. | |
| +| **D, L, R, Y, DD–NN: session ownership** | `ownSession` checks mode, intent, both account identifiers, known customer and bound ID (`billingservice.go:273`). Purchase checks follow ownership. Expected-intent binding checks its recorded account before deriving an account from the response (`:1204`). Create/list recovery cannot authorize another account’s session. | |
| +| **Bound-ID expiry exception** | A previously bound session with contradictory identity is reviewed first, then its **recorded bound ID** may be expired (`billingservice.go:310`; `billingretirement.go:914`). That is distinct from accepting its contradictory response as proof. The response cannot retire the intent. This is the justified exception to refusing expiry of an unowned create/listing result; leaving a disclosed payable bound URL open solely because metadata changed would be the worse authority choice. | |
| +| **Reconcile versus binder** | A first purchase must satisfy matching session purchase identity and its own subscription association (`billingservice.go:997`, `:1038`, `:1054`). Complete-with-no-subscription and pending payment retry. Renewals do not undergo first-purchase intent consumption. Refused matching-intent subscriptions are retained before rejection. Unknown terms outside the bound-first-purchase case remain retryable; an open review/disputed record can also continue returning conflict/503 under the explicitly conservative policy. | |
| +| **X, AA, CC: quantity history** | `quantityOutcomesSQL` supplies the processing guard, startup detection and obligation check (`billingquantityaudit.go:20`, `:42`, `:61`). The healing sweep is gone; startup does not clear pre-history or clock-rollback gaps. | |
| +| **Z, BB, YY, ZZ, BBB, CCC: operator records** | References and resolution outcomes are retained. Historical omitted outcomes remain `retire`; exact replay compares actor, note and outcome. The rejected listing is operator-only, newest first and bounded to 200. It drains rows before account lookups (`billingoperator.go:290`). It is not a universal ledger of interactive errors or retrying events. The added migration and ordering tests establish their fixtures, not every concurrent execution. | |
| + |
| +The lifecycle invariant supported by the code is: **durably classified named subscriptions survive intent consumption as accepted-purchase identity or disputes; expiry and cleanup preserve an intent with recorded obligations or an open refusal; operator retirement checks the recorded proof and obligations transactionally.** It is not yet a complete invariant across authoritative account retirement or all decoded listing evidence. |
| + |
| +There are four production intent-delete statements and one binding helper: |
| + |
| +| Door | Effect on previously recorded evidence and later observations | |
| +|---|---| |
| +| **First-purchase record — `billingstore.go:890`** | Exact-intent deletion, account/assignment changes, event, entitlement queue, consumption archive and disputes for other subscriptions commit together. Failure rolls everything back. Later subscription observations compare with that archive. Same-subscription renewal/cancellation consumes no intent. | |
| +| **Ordinary expiry — `billingstore.go:553`** | Reached through pass, click and refused-session expiry, including unbound refused creation. Proof is checked first. Known naming subscriptions cause a refusal and preserve the intent; open review/dispute blocks deletion. | |
| +| **Unready cleanup — `billingstore.go:604`** | Reached through pass, replacement click and account-deletion preparation. `customer_ready=0` plus no session distinguishes never-created checkout. Recorded subscriptions or open review prevent cleanup. | |
| +| **Operator retire — `billingstore.go:1203`** | Resolves and archives exactly the reviewed intent, with proof, outcome and quantity-event disposition, in one transaction. Protocol-zero reviews, disputes and attached subscriptions remain held. | |
| +| **Session binding — `billingstore.go:427`** | Both binding callers use this helper. It refuses another purchase’s session and a session conflicting with an open review reference. Same-session rebinding and lost-answer recovery remain possible. | |
| +| **Account retirement** | No intent cascade exists; local soft deletion and reaping check billing holds. The reservation-to-tombstone interval remains defective as Finding 1 describes. | |
| +| **Webhook settlement** | Required classification precedes acknowledgement. Evidence and rejected/seen-event recording use separate transactions; a crash between them leaves the event retryable. Archived identity paths retain attribution. Neither acknowledged rejection nor event deduplication independently resolves a review. | |
| + |
| +For observations committed before expiry or cleanup, the transaction ordering retains them. If deletion wins first, the explicitly documented exception applies: **ordinary expiry or unready cleanup with no previously recorded naming subscription leaves no archive**, and a subsequently observed subscription can become only a rejected event. Consumption and operator retirement have archives and do not share that exception. |
| + |
| +`billing_consumed_intents` is written when an intent is actually consumed. Generated intent IDs make ordinary replacement of an existing archive unnecessary; deliberate historical ID reuse is not a supported lifecycle. The account’s recorded subscription and the archive’s accepted subscription exclude ordinary renewal, cancellation and redelivery from becoming disputes. The empty-session fallback is now consistent. |
| + |
| +For **stage-one retirement**: |
| + |
| +- Fate is written once; contradictory terminal/identity evidence creates a durable dispute. |
| +- Review protocol, attachments, disputes and fate are checked inside the resolution transaction. No caller’s stale generation authorizes release. |
| +- `keep` requires the bound, owned, matching, completed and acceptably paid session. It does not fully validate the subscription’s terms; subsequent reconciliation can reopen the refusal, as the governing “as built” section states. |
| +- A kept intent receiving another subscription is held again; consumption disputes additional recorded subscriptions. Late session refusals now survive ordinary consumption. |
| +- A dispute has no stage-one resolution. An old client omitting outcome cannot bypass that rule. |
| +- Expiry failure or process death after filing leaves the review authoritative. `advanceRefusal` retries the pinned bound session and obtains proof through reads; a winning completion remains held. |
| +- The refusal turn has its own 45-second deadline, three-attempt budget and longest-due ordering. Both turns inherit caller cancellation. This avoids the reported fixed-order starvation, without promising bounded completion for an arbitrary workload. |
| +- Trigger recreation is transactional. A failed startup retains the previous committed trigger. The trigger alone does not make an older binary obey attachment and account-retirement rules. |
| + |
| +For **MMM–SSS**, the inspected production checkout/subscription reads use `readCheckout`, `listCheckouts` and `readSubscription`: start/resume, recovery, binder, reconciliation, pass, keep gate, refusal advancement, quantity worker and invoice checks. Store failure returns no usable object. The meaningful pre-classification returns are provider errors, requested-ID/mode mismatches and the explicitly excluded unidentifiable/no-refusal states; Finding 2 shows provider decoding errors can conceal successfully decoded neighboring observations. |
| + |
| +The structural test at `billingevidence_test.go:544` catches selector uses through aliases, method values, method expressions, interfaces and same-named methods on another type. Reflection also requires classification of every exported provider-client method. Its recording-order check is lexical, not control-flow proof; an unreachable recording call could satisfy it. A differently named read on an unrelated client remains outside that inventory. |
| + |
| +For **I, P, LLL, RRR, ZZZ2 and JJJJ**, the source supports the targeted counterfactual tests, with these limits: |
| + |
| +- The four previously surviving round-20 mutations now have distinguishing cases: individual identity fields, an incomplete additional subscription reopening after `keep`, the refusal’s sole read recording fate, and contradiction only on the second resume read. |
| +- The refusal-attachment rollback test injects failure inside the production helper, rather than merely testing an unrelated SQLite transaction. |
| +- The fake’s distinct second session and retired-completion replay assertions distinguish recording a retired purchase. |
| +- The historical-retirement helper recreates material pre-protocol state; it is not permission for current code to produce it. |
| +- Review-three/four qualifications matter: provisioning assertions inspect shell text; successful migration does not demonstrate every crash ordering; account-only deletion can be redundant behind an exact-intent lookup in the same transaction; removing an exactness assertion over exact data is not a distinguishing mutation. |
| +- The new late-filing test and other store-sequenced tests establish specific boundaries, not two concurrently running consoles. I cannot certify the claimed executed mutation counts or exact transformations from their descriptions. |
| +- I found no new shared-cookie-jar, temporary-root or calendar-dependent assertion defect. Fixture timestamps and provider hooks do not establish real provider ordering. |
| + |
| +For **BBBB and LLLL**, I found no additional regression relative to `b260ac8e` in an honest-provider sequence established without the disputed early-subscription/replacement assumptions: |
| + |
| +| Sequence | Result | |
| +|---|---| |
| +| Own subscription arrives before the session names it | Retained; reconciliation retries, then records once association/payment checks pass. | |
| +| Duplicate delivery | Idempotent attachment/event handling; no second subscription obligation. | |
| +| Payment retries on the same session and subscription | Pending until acceptable payment and identity; then ordinary recording. | |
| +| Abandonment leaves a named subscription and the session expires | Held without a stage-one disposition, even if the subscription later reads `incomplete_expired`. | |
| +| Provider replaces one subscription with another for the intent | The extra recorded ID becomes a dispute when the accepted purchase records. | |
| +| Renewal/cancellation after recording | Does not consume a newer intent or dispute the purchase merely for being a later event. | |
| + |
| +Whether the problematic honest sequences occur, and what their financial consequences are, remains a provider rehearsal and settlement-policy question. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +1. **All 36 catalog identities:** exact integer amounts, currencies, metadata, tax behavior and expanded own-only `currency_options`. Console and provisioning checks must accept the same legitimate shapes and reject mutations. |
| +2. **One-form purchases:** India plus explicit GBP, country-default choices, cleared suggestions and held legacy EUR. Confirm displayed base price, checkout currency, disabled Adaptive Pricing, subscription/item currency and stored identity. |
| +3. **Payment/event ordering:** subscription before completion, completion before subscription attachment, duplicate delivery, declined/retried cards, abandoned 3-D Secure and delayed payment methods. Establish how many subscription IDs Stripe creates and when. |
| +4. **Financial obligations:** determine what `incomplete_expired` and cancellation leave outstanding—payments, collectible invoices, pending collection and refunds. No status alone is settlement. Apply the owner’s ruling before enabling any release that depends on it. |
| +5. **Expiry versus payment:** retained links, expiry failure/timeouts, payment racing expiry and subsequent GET visibility. Proven expiry must prevent further payment; winning completion must remain accounted for. |
| +6. **Identity and idempotency:** GET/list/create/expiry fields, metadata correction, concurrent identical keys, lost responses, remembered errors and key-retention expiry. Confirm session IDs are not recycled. |
| +7. **Lost-create recovery:** original-key replay and customer-scoped pagination, including failures. Recovery must offer no link from a failed listing while retaining attributable evidence. |
| +8. **GBP overages and quantity:** creation, lost-response recovery, item/final-line verification, finalization/payment and quantity/proration updates must preserve GBP and correct allowance dating. |
| +9. **Legacy/customer reuse:** unchanged legacy EUR request shapes and hashes, renewal/cancellation, subsequent purchases in the held currency, and unchanged historical invoices. |
| +10. **Ended subscriptions:** establish whether fields other than metadata can change after cancellation, separating the defensive regression fixture from actual provider behavior. |
| + |
| +**MMMM:** acceptance still requires the two code fixes above. What remains is therefore **not only** the listed decisions, rehearsal and owner approvals. |
| + |
| +**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 0584fcc8b..0b35d7df1 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -470,7 +470,7 @@ Four of the eight findings are one fault seen from four sides: a subscription th |
| | Astra 5: the refusals' turn took intents in the order they stand, so older ones could use the budget for ever. | Fixed: the longest-due first. | `billingretirement.go` `billingPassRefusals` | `TestTheRefusalsTurnTakesTheLongestDueFirst` | |
| | Claims: "nothing read about a refused purchase is dropped"; "never by what the answer claims"; "nothing retires under an unbound review"; "an intent is never gone at its resolution"; "a dispute and never nothing"; the create answer's exception; the review-twenty row on an unbound intent. | Corrected. The header and the design state the rule, what it covers and what is lost outside it, including the one exception for a create answer that shows a completion. An on-terms subscription naming an unbound intent is a retry, and is now recorded at the read. | header, design, comments | | |
| -Found while answering, by neither reviewer, and of the same family as review eighteen's P2: **a later event of an already recorded subscription deleted whatever standing intent its metadata named.** The record's delete of the intent ran for every state change, not only a first purchase; a subscription that had ended, with its metadata made to name a newer purchase's standing intent, deleted that intent while its session could still be paid. Fixed: only a first purchase consumes an intent, and whether a record is a first purchase is judged in the record's transaction from the account's own row (the subscription is not yet the recorded one), not from what the caller read (`billingstore.go` `applyBillingStateDated`; `TestALaterEventOfARecordedSubscriptionConsumesNoIntent`). |
| +Found while answering, by neither reviewer, and of the same family as review eighteen's P2: **a later event of an already recorded subscription deleted whatever standing intent its metadata named.** (A defensive fix: reaching it needs the provider to change something other than metadata on a subscription that has ended, which the test does through the fake by moving its period, and which the provider is not known to do; it is on the test-mode list.) The record's delete of the intent ran for every state change, not only a first purchase; a subscription that had ended, with its metadata made to name a newer purchase's standing intent, deleted that intent while its session could still be paid. Fixed: only a first purchase consumes an intent, and whether a record is a first purchase is judged in the record's transaction from the account's own row (the subscription is not yet the recorded one), not from what the caller read (`billingstore.go` `applyBillingStateDated`; `TestALaterEventOfARecordedSubscriptionConsumesNoIntent`). |
| Existing tests that changed, and why: `TestCheckoutAfterAPurchaseKeepsItsCurrency` (the fake reused one session id for two purchases, which the bind now refuses; the second checkout has its own id); `TestEvidenceAgainstARecordedFateIsADispute` (an expiry proof for an intent a subscription names now files the review again; the dispute clause is tested with that taken away by hand); `TestBillingStateAtomicEventAndRevision` and `TestBillingIntentPinsCustomerAndCoupon` (hand-built requests now name the customer the account has, as the start path does). |
| @@ -481,9 +481,50 @@ The full `internal/console`, `internal/plans` and `internal/billingstripe` packa |
| Stated and not changed: |
| - A subscription first seen naming an intent that already expired in the ordinary way, with nothing seen naming it before, is a rejected event with its account and no hold. Nothing is kept of such an intent. |
| -- A subscription seen naming an intent holds that intent's expiry even when the subscription is dead (`incomplete_expired`). The buyer then waits on a person. Whether the provider ever makes a subscription before its checkout session completes decides how often an honest buyer can meet this; it is on the test-mode list. |
| +- A subscription seen naming an intent holds that intent's expiry even when the subscription is dead (`incomplete_expired`). The buyer is then held with no disposition in stage one: `retire` is held while anything is attached and `keep` needs a completed session, so no person can end it until what settles such a case is decided (decisions 1 to 4, and a new one: may a subscription the provider reports `incomplete_expired`, with no paid invoice, be treated as nothing owed). Whether the provider ever makes a subscription before its checkout session completes decides whether an honest buyer can meet this at all; it is a check to make in test mode before billing is enabled, and a gate on enabling. |
| - A dispute made at a record holds the account's invoices and has no resolution in stage one, like every dispute. |
| - The refusals' budget is still three a pass; fairness is by due time, and there is no cursor. |
| - The structural guard's check that a wrapper records before it returns is lexical, as Astra says. The behavioural tests and the mutants are what show each wrapper records. |
| - No test runs two console processes. The two-console findings are tested through the store's transactions, one after another. |
| +## Review twenty-two (reviewed `05ccec07`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review22.md`, `CODEX-FINDINGS-multi-currency-review22.md`. Both found every round-twenty-one finding closed and each named test failing without its fix. Opus: the named-subscription rule holds as written, review eighteen's P2 is closed for deletion and consumption, one P2 (what the rule costs an honest buyer when the provider leaves a dead subscription), four P3s. Astra: three P2s at the rule's edges (account deletion; a session-only refusal filed late; a click racing a filing) and one P3. **Review eighteen's P2 stays recorded as open until a round accepts this code.** |
| + |
| +| Finding | Disposition | Where | Counterexample test | |
| +| --- | --- | --- | --- | |
| +| Astra 1 (P2): an account could be retired while a checkout dispute, or a refusal whose intent is gone, was open on it; its reviews were then listed under no account and resolved through no door. | Fixed: the retirement check refuses while a checkout refusal or a checkout dispute is open, in the transaction that reserves, deletes and reaps. | `billingstore.go` `checkBillingRetirement` | `TestAnAccountIsNotRetiredUnderAnOpenCheckoutDisputeOrRefusal` (with the control: an honest purchase that ended retires) | |
| +| Astra 2 (P2): a console that judged a session not the purchase and filed its refusal after another console recorded the purchase lost the refusal: the gone-intent branch had no subscription to classify and the session's purchase was not in any archive it searched. | Fixed: a refusal of a session filed for an intent that a purchase consumed is kept as a dispute against what was consumed, in the filing's transaction, whatever subscription it carried. | `billingstore.go` `fileCheckoutRefusal` | `TestARefusalOfASessionFiledAfterItsPurchaseRecordedIsKept` | |
| +| Astra 3 (P2): a click that passed its first open-review check could return a pay link for an intent whose refusal another console filed while the click's read was in flight; the post-read checks looked at the dispute only, and a newly made session's link was disclosed with no check at all. | Fixed: after each read the state is read again and an open review or a dispute stops the click; a new session's link is disclosed only if the intent is not under review then (it stays bound, for the pass to expire). | `billingservice.go` start path | `TestAClickOffersNothingWhenAReviewOpensWhileItReads` (first read, second read, new session) | |
| +| Astra 4 (P3): a page that repeated a session was refused before the repeat, and what followed it, was returned, so a second answer about an archived session was lost. | Fixed: the client returns everything the page decoded with the pagination error; the wrapper records it all. | `billingstripe/catalog.go`, `billingretirement.go` `listCheckouts` | `TestABrokenListingStillRecordsBothAnswers`; `TestListCheckouts_ABrokenPageStillReturnsWhatItSaid` | |
| +| Opus P2-1: an honest buyer whose provider left a dead subscription naming the intent (an abandoned authentication; a declined card then another) is held with no stage-one disposition; the dispositions said "waits on a person", and no person can act. | **Not changed in code; made a gate and a decision.** The code does not read an attached subscription's status, and whether `incomplete_expired` is nothing owed is a ruling (the test-mode list already asks what it leaves outstanding). Whether the provider ever makes a subscription before its session completes is the first check of the test-mode rehearsal; if it does, that ruling is needed before billing is enabled. The dispositions and the header say so. | `DISPOSITIONS`, header, `DECISIONS-FOR-PAUL.md` on the launch branch (decision 15; `ENABLEMENT.md` section 6 step 2) | | |
| +| Opus P3-1: the rule's stated exception (an intent that expired in the ordinary way) was not the only one: an unready intent cleaned up with nothing naming it leaves nothing either. | Stated, in the header and the design. | header, design | | |
| +| Opus P3-2: a subscription was classified only under the account its own metadata named; naming A's intent under B, or no account, it was recorded against nothing. | Fixed: the account is the one the console recorded the intent for (standing, consumed, retired, or resolved), by the intent id alone; the metadata's account is used only for an intent the console has no record of. | `billingretirement.go` `noteSubscriptionSeen`, `accountOfIntentTx` | `TestASubscriptionIsRecordedUnderTheIntentsOwnAccount` | |
| +| Opus P3-3: the pass reported a hold it caused as a failure, once. | Fixed at both sites (`refuseSession` through the pass; recovery). | `billingpass.go` | `TestThePassDoesNotFailOnAHoldItCaused` | |
| +| Opus P3-4: the record and the classifier used different keys for a dispute about an intent consumed with no session. | Fixed: the intent id in both. | `billingretirement.go` `classifySubscriptionTx` | `TestADisputeAboutAConsumedIntentWithNoSessionIsKeyedByTheIntent` | |
| +| Opus claims 1 to 3. | 1 corrected (two exceptions). 2 corrected (held with no disposition). 3 corrected (a defensive fix, reachable through the fake). | | | |
| + |
| +Mutation checks, in a tree copy, nine, each run against the round's tests and the clicks': an account retired under an open checkout review; a refusal filed after the record lost; the first read's check, and the second's, seeing only a dispute; a new link disclosed under review; the client dropping a broken page; a subscription recorded under the account its metadata names; the pass failing on the hold its refusal caused; a dispute about a consumed intent with no session being nothing. One survived the first run: the first read's check, which the second read's check shadows for a click that changes nothing; the test's first-read case is now a click that changed its country, which the first check stops before the click would expire the session under its review (and asserts no expiry was requested). All nine die. Output: `currency-mutants-round22.txt` in the run directory's logs. |
| + |
| +Stated and not changed: everything under review twenty-one's "Stated and not changed", with the second bullet as corrected above; the structural guard's wrapper check is lexical; no test runs two console processes. |
| + |
| + |
| +## Review twenty-three (reviewed `2618403f`; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review23.md`, `CODEX-FINDINGS-multi-currency-review23.md`. Opus: every round-twenty-two finding closed or honestly stated, no P1 or P2, three P3s and four claims; "on that basis I would count review eighteen's P2 as closed at `2618403f`". Astra: one P2 and one P3. **Review eighteen's P2 stays recorded as open until a round ends with both accepting**; Opus's sentence is noted and is one reviewer's. |
| + |
| +The gate on `2618403f` **failed** (`gate-currency-2618403f.txt`, exit 2): one existing test, `TestABoundSessionReadBackAsNobodysIsReviewed`, expected a retire held for an unproven expiry and got it held for an attached subscription. That is round twenty-two's Opus P3-2 at work (a subscription naming the intent under another account's name is now recorded against the intent), and the test's expectation was out of date; it now asserts both holds, the attachment first and the unproven expiry once the attachment is taken away by hand. The test was not in the subset run before the commit; the full package run is part of the round's checks again. |
| + |
| +| Finding | Disposition | Where | Counterexample test | |
| +| --- | --- | --- | --- | |
| +| Astra 1 (P2): a dispute committed after the retirement's reservation and before the account's tombstone was published did not stop the publication; the deletion then refused at its last step, leaving an account retired in the deployment's manifest and present here, under a dispute stage one cannot resolve. | Fixed by a point of no return in the database: `commitBillingRetirement`, taken after the reservation and before the first write outside, re-reads every condition in its own transaction and marks the reservation committed. Filing a checkout refusal or a checkout dispute withdraws a reservation that is not committed, in the evidence's own transaction. SQLite serialises the two: either the evidence withdraws first and the commit refuses (nothing outside was touched; the reservation is released), or the commit is first and the evidence lands on an account whose retirement finishes (the deletion and the reap pass a review that opened after the commit). | `billingstore.go` `commitBillingRetirement`, `cancelPendingRetirementTx`, `fileBillingReviewTx`, `checkBillingRetirement`; `ownership.go` the deletion handler, `DeleteAccount`; `store.go` `ReapAccount` | `TestARetirementIsCommittedInTheDatabaseAndEvidenceSerialisesAgainstIt` (three cases) | |
| +| Opus P3-2: evidence first seen after an account was deleted landed where no operator looked (the listing left the account blank; the door answered 404). | Fixed: the operator doors look accounts up whether or not they are deleted; the listing marks such a row `account_deleted`. | `billingoperator.go`; `billingstore.go` `accountEvenIfDeleted` | the same test, second case | |
| +| Astra 2 (P3): one session on a listing page that did not decode discarded the page's other sessions, one of which could be a contradiction of an archived fate. | Fixed: the client decodes each listed session on its own and returns the ones that decoded with the decode error. | `billingstripe/catalog.go` | `TestListCheckouts_AMalformedSessionKeepsTheOthersOnItsPage` | |
| +| Opus P3-1: the late-refusal dispute used a third key for an intent consumed with no session. | Fixed: the intent id, as the record and the classification. | `billingstore.go` `fileCheckoutRefusal` | | |
| +| Opus P3-3: the recovery site's filter of the hold it caused had no test. | Tested. | | `TestThePassDoesNotFailOnAHoldItsRecoveryCaused` | |
| +| Opus claims: "discarded unconditionally" (`resolveIntentForDeletion`); "an hour later at the earliest"; the dispositions' "Where" naming a file not in this branch; the enablement plan's "costs an honest buyer nothing". | The two comments corrected; the "Where" reads `DECISIONS-FOR-PAUL.md` on the launch branch; the plan's sentence is qualified there (two subscriptions naming one intent by any means, not only a subscription before completion). | | | |
| + |
| +Mutation checks, in a tree copy, eleven, all die: checkout evidence not withdrawing a pending retirement; the commit not re-reading the conditions, and not needing a reservation (each needed its own test case, added); the deletion refusing a review opened after the commit, and ignoring reviews whether or not committed; the listing leaving a deleted account blank; the door answering 404 for one; a committed reservation withdrawn; a session that does not decode dropping its page; the recovery's hold the pass's failure; the late refusal of a session not kept. Output: `currency-mutants-round23.txt` in the run directory's logs. |
| + |
| +Stated and not changed: the reap of an account the manifest already records as retired passes an open checkout review (the review stays listed and resolvable under the deleted account); the structural guard's wrapper check is lexical; no test runs two console processes (the serialisation is tested through the store's transactions in sequence, which is what SQLite's one writer makes of two consoles). |
| + |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review22.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review22.md| new file mode 100644 |
| index 000000000..d24c74ba7 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review22.md |
| @@ -0,0 +1,183 @@ |
| +# Twenty-second review of `feat/carloku-multi-currency-20261001` at `05ccec07` |
| + |
| +This is a source-only review: I read files with `git show 05ccec07:` (base `12e70d95`, delta `b260ac8e..05ccec07`). I modified nothing and ran nothing. The pricing site at `01b62f7` has not changed since the last review. |
| + |
| +**Verdict up front: accept with fixes.** All eight of round twenty-one's findings are closed, and each named test would fail without its fix. The named-subscription rule holds as written: no path I found deletes or consumes an intent past a recorded subscription, or releases a hold with one attached. The one P2 is what the rule costs an honest buyer when the provider leaves a dead subscription. The code then holds the buyer in a state no stage-one outcome can end. The dispositions say this buyer "waits on a person", but no person can act. |
| + |
| +## Round twenty-one, finding by finding |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P2-1 / Astra 1**: an unready intent under review was cleaned up, then `retire` released it | **Closed**, in three places. `discardUnreadyIntent` refuses an open review in its own DELETE (`billingstore.go:604`). `held()` checks protocol, dispute and attachments before asking whether the intent stands (`billingretirement.go:251-268`). The barrier no longer exempts an unready intent under an open review, and is rebuilt at every start. | **No.** `TestAnUnreadyIntentUnderReviewIsNotCleanedUp`: the pass at +26h deletes the intent and `standing` fails. With only the barrier present, the pass returns the trigger's abort and the test fails on the pass error. | |
| +| **Astra 2**: a subscription seen before any refusal was lost when the honest purchase consumed the intent | **Closed.** `classifySubscriptionTx` attaches with or without a review (`:594`). The record archives what it consumed and disputes every other attached subscription in its own transaction (`billingstore.go:911-938`). A later or late-filed subscription is compared with `billing_consumed_intents` (`billingretirement.go:637-645`). | **No.** All three subtests count disputes, and the old code makes none. | |
| +| **Opus P3-2**: a hand-made subscription on a bound intent with an open session was retried, then retired on expiry | **Closed.** It is recorded at the read. `retireIntent` → `holdForNamedSubscriptionsTx` files the refusal with the session as reference, and the intent stands (`billingstore.go:527-538`). | **No.** In `TestAnExpiryDoesNotRetireAnIntentASubscriptionNames` (pass and click), the old code retires the intent. | |
| +| **Astra 3**: stale customer or held currency when creating an intent | **Closed** (`billingstore.go:393-398`). The start path's `request.Customer` is `state.Customer`, and `billing_accounts.customer` is written only by the record. So a stale request fails once and the next click reads fresh state. No honest request is refused for good: house accounts, accounts with no row, and resumes all pass the account's own customer and terms. | **No.** The first `beginBillingCheckout` would create the intent. | |
| +| **Opus P3-1**: a bind onto another session or an archived one | **Closed.** `bindIntentSession` (`billingstore.go:434-475`) is the only writer of `session_id`. It refuses a session that is another purchase's (`sessionBelongsElsewhereTx`, consumed included) or that is not the session the open review names. Legitimate rebinds still pass: the same session again; recovery after a lost create; a review that names no session. | **No.** All three halves assert an identity refusal. | |
| +| **Opus P3-3**: the store keyed on the answer's id | **Closed** (`billingretirement.go:793-797`). Every caller treats a mismatched id as a failed read: explicitly, or through `ownSession`'s bound-id clause, or through `ownBoundSession`'s `session.ID != intent.SessionID`, which returns false without filing. | **No.** | |
| +| **Astra 4**: a listing's earlier pages were lost on a later page's failure | **Closed** in both the client and the wrapper. The only other caller of the client's listing is `catalog_test.go`, which checks the error. | **No**, for either half. | |
| +| **Astra 5**: the refusals turn was unfair | **Closed.** It sorts by `NextTry` ascending. A refusal that stays due keeps its old `NextTry`, so it rises to the front within a few passes. | **No.** In order of standing, the test would work `ci_fair_1..3`. | |
| +| **Opus claims 1–4** | 1 and 2 are corrected (the pass comment is now true). 3 is stated in the dispositions. 4 (the site's source-text assertion) is unchanged and acceptable for the reason given last round. One residual overstatement remains: see Claims, item 1. | n/a | |
| +| **Astra claims** | Design line 125, the operator comment, "a dispute and never nothing" and the listing claim are all corrected. The create-answer exception is now documented in the header. | n/a | |
| + |
| +**The defect found while answering** (a later event of a recorded subscription deleted the standing intent its metadata named): |
| +- **Real in the code at `b260ac8e`.** The delete at `billingstore.go:776` ran on every state change of the recorded subscription. |
| +- **Probably not reachable through Stripe.** It needs a new intent standing, which requires the recorded subscription to be `canceled` or `incomplete_expired`. It also needs a later *state change* of that same subscription, because the no-op path returns before the delete. To my knowledge Stripe changes nothing on a canceled subscription but its metadata, and metadata is not part of the comparison. |
| +- **The test reaches it only through the fake.** It moves a canceled subscription's period (`current_period_start: f.now`). |
| +- **Closed.** `current.Subscription != next.Subscription` is judged in the transaction (`billingstore.go:868`). The test fails without the fix. It is a sound defensive fix. Add "a canceled subscription's fields never change" to the test-mode list. |
| + |
| +**Where the reviewers disagreed on severity last round, the code sides with Astra:** |
| +- **Astra 2 at P2, against my P3-2.** At `b260ac8e`, once the honest C recorded, H's event hit `state.Subscription != "" && active → errBillingIdentity` (`billingservice.go:977`). That is a bare rejection with no account and no hold, while H billed at Stripe. That is a release of a case with a subscription outstanding. |
| +- **Astra 3 at P2.** I did not report it. It lets a payment be taken that can never record. |
| + |
| +## Findings |
| + |
| +### P2-1. When the provider leaves a dead subscription, an honest buyer ends in a hold that no stage-one outcome ends |
| + |
| +**Where:** |
| +- `billingretirement.go:594` attaches every subscription naming a standing intent, whatever its status. |
| +- `billingretirement.go:662-681` (`holdForNamedSubscriptionsTx`) turns any attachment into a refusal at expiry or cleanup. |
| +- `billingretirement.go:263` holds `retire` while anything is attached. `:283` holds `keep` unless the fate is complete. |
| +- `billingstore.go:915-938` disputes, at the record, every other attached subscription. A dispute has no resolution in stage one. |
| +- `billingservice.go:614-618`: the start path refuses under any open review. |
| + |
| +**Scenarios.** Both depend on Checkout creating a Subscription before its session completes. The dispositions put that on the test-mode list. |
| +1. **3-D Secure abandoned.** |
| + 1. The buyer confirms, and the provider creates C in `incomplete`, naming I through `subscription_data.metadata`. |
| + 2. `customer.subscription.created` → `readSubscription` attaches C to I. Reconcile reads the session `open`, so the result is `errBillingPending`. |
| + 3. The buyer abandons. C goes to `incomplete_expired`, and the session expires at 24h. |
| + 4. The pass's `expireResolvedIntent` → `retireIntent` → hold, which files the refusal referencing S. `advanceRefusal` then proves `expired`. |
| + 5. **Result:** |
| + - `retire` is held, because C is attached. |
| + - `keep` is held, because the session is not complete. |
| + - The start path answers "under review" on every click. |
| + - Account deletion is blocked, because the intent stands. |
| + |
| + No operator outcome exists, and C never collected anything. |
| +2. **Card declined, then a second card on the same session.** |
| + 1. If each attempt makes a subscription, C₁ (dead) and C₂ (active) both name I. |
| + 2. C₂ records and, in the same transaction, disputes C₁. |
| + 3. **Result:** the account's invoices are held for good, with no stage-one resolution. |
| + |
| +**What the code can tell apart today:** nothing. It never reads the status of an attached subscription. A subscription that is `incomplete_expired`, with no paid invoice, provably collected nothing. A hand-made active subscription did. The other honest sequences end with no hold: |
| +- The subscription event before the completion: it is attached, then excluded at the record as the purchase's own. |
| +- A duplicate delivery: `INSERT OR IGNORE` and the event dedupe absorb it. |
| +- A delayed payment method: the session is complete, the intent stays pending, and nothing expires. |
| + |
| +**Smallest acceptable fix:** make this a gate before enabling billing. Run the test-mode checks below. |
| +- If Checkout never creates a subscription before completion, correct the dispositions' "waits on a person" to "is held with no disposition in stage one", and leave it there. |
| +- If it does, read each attached subscription inside the hold and the record's dispute loop (through `readSubscription`). One that reads `incomplete_expired` neither holds an expiry nor makes a dispute. |
| + |
| +### P3-1. The rule's stated exception is not the only one |
| + |
| +**Where:** the header at `billingretirement.go:91-94` and the design's "Named subscriptions" bullet name only an intent that "expired in the ordinary way". |
| + |
| +**Scenario:** |
| +1. An unready intent, with nothing seen naming it, is discarded by the pass at +25h, by the start path, or by the preparation of an account's deletion (`billingstore.go:604`). Nothing is archived. |
| +2. A hand-made H naming it is read afterwards. `classifySubscriptionTx` finds no archive and returns nil. |
| +3. Reconcile settles H as a rejected event with its account. |
| + |
| +That is the same loss as the stated exception, by a path the text does not name. |
| + |
| +**Fix:** name it in both places, or archive discarded intents as consumption does. |
| + |
| +### P3-2. A subscription is classified only under the account its own metadata names |
| + |
| +**Where:** `billingretirement.go:703-717`. |
| + |
| +**Scenario:** |
| +1. H is made by hand with `checkout_intent=I` (account A's intent) and `account_sqid` missing, or set to B. |
| +2. Nothing is attached to I. Reconcile answers a bare identity refusal. |
| +3. A's session then expires in the ordinary way, and I goes with nothing held. |
| + |
| +Intent ids are globally `UNIQUE` (`billingstore.go:124`), so the store could find I's own account from the id alone. |
| + |
| +**Fix:** classify by the intent id against the account the console recorded for it, or state the limit. |
| + |
| +### P3-3. The pass reports a hold it caused as a failure, once |
| + |
| +**Where:** `billingpass.go:267` passes through `refuseSession`'s `errBillingUnderReview`, which comes from `retireIntent`'s hold. `recoverReadyIntentSession`'s refusal does the same. `expireResolvedIntent` (`:322`) already filters this error. |
| + |
| +**Effect:** one logged pass failure per such hold. No state is wrong. |
| + |
| +**Fix:** filter `errBillingUnderReview` at both sites. |
| + |
| +### P3-4. The record and the classifier disagree about a consumed intent with no session |
| + |
| +**Where:** the record keys its disputes on the intent id when `usedSession==""` (`billingstore.go:930-933`). `classifySubscriptionTx` returns nil for the same case (`billingretirement.go:641`). |
| + |
| +**Effect:** latent. No production path consumes an unbound intent, because reconcile requires `intent.SessionID != ""` before it records. |
| + |
| +**Fix:** use the same key in both places. |
| + |
| +## AAAA–JJJJ in brief |
| + |
| +**AAAA, the deletion paths.** There are four statements that delete an intent, and one writer of `session_id` (`bindIntentSession`). For each deletion, what happens to a subscription seen naming the intent: |
| + |
| +| Deletion | Before | During | After | |
| +|---|---|---|---| |
| +| Record (`:890`) | Disputed at the record | Disputed at the record | Disputed against `billing_consumed_intents` | |
| +| Ordinary expiry (`:553`) | Held | Held | Stated exception | |
| +| Unready cleanup (`:604`) | Held | Held | Unstated exception (P3-1) | |
| +| Operator retire (`:1203`) | Held | Held | A dispute when the fate is `expired` | |
| + |
| +- **"During"** with two consoles: WAL with deferred transactions turns a stale read-then-write into `SQLITE_BUSY_SNAPSHOT`, so the two transactions serialize; they never interleave. |
| +- **Operator retire with fate `''`:** the only retire with fate `''` is an unready intent with no review reference, and any review on one has something attached, so retire is held. |
| +- **No cascade:** accounts are soft-deleted, and `checkBillingRetirement` refuses while an intent stands. |
| + |
| +**BBBB, the cost to an honest buyer.** See P2-1. |
| + |
| +**CCCC, `billing_consumed_intents`.** |
| +- It is written only at `n==1` of a first-purchase record. `INSERT OR REPLACE` is harmless, because intent ids are unique and the row is deleted. |
| +- Classification order: |
| + 1. what was already attached → nil; |
| + 2. the retirement archive; |
| + 3. the kept resolution; |
| + 4. the consumed record. |
| +- The account's own renewals and cancellations are excluded as the recorded subscription. A new purchase's subscription names its own intent. Neither becomes a dispute. |
| + |
| +**DDDD, only a first purchase consumes.** Reconcile is the only production caller with `consumeIntent`. Its flag equals the store's judgement under the revision check. A first purchase cannot record without consuming. |
| + |
| +**EEEE, binding.** The callers are the binder and `saveBillingCheckout` (the create, recovery and resume paths). The refusals are correct. The design assumes the provider never reuses a session id. |
| + |
| +**FFFF, creating an intent.** See Astra 3 in the table. |
| + |
| +**GGGG, the barrier.** |
| +- The DROP and CREATE run in one transaction, so a crash leaves the old text, never none. |
| +- An older binary's `CREATE TRIGGER IF NOT EXISTS` leaves this build's text in place. |
| +- Two consoles starting together serialize, or one fails its start and restarts. |
| +- The barrier does not cover an intent with something attached but no review, which an older binary would delete. The roll rule (stop older consoles first) covers that. |
| + |
| +**HHHH, reads and listings.** Correct. |
| + |
| +**IIII, the refusals' turn.** No starvation. Throughput is three per pass, as stated. |
| + |
| +**JJJJ, the tests.** Each new test drives the production door it names and would fail without its code. The changed fixtures are justified: |
| +- `TestCheckoutAfterAPurchaseKeepsItsCurrency`: the fake now gives the second checkout its own session id. |
| +- `TestEvidenceAgainstARecordedFateIsADispute`: the hold is removed by hand to test the dispute clause alone. |
| +- Hand-built requests now name the customer the account has, as the start path does. |
| + |
| +None runs two console processes, as the dispositions say. I judged the mutants from source, and each named test reaches its line. |
| + |
| +## Sections A–K, L–ZZZ2 and the pricing site |
| + |
| +The delta touches no pricing, form, quote, cookie, trigger, export or site code. Review eighteen's P2 (DDD) is **closed** for deletion and consumption: no path I found removes an intent while its session is payable, or releases a hold with a known subscription. |
| + |
| +The site at `01b62f7` is unchanged, so last round's conclusions on K, MM and QQ stand. |
| + |
| +## Claims the code contradicts |
| + |
| +1. The header (`billingretirement.go:91`) and the design's "What stays outside" name one exception; there are two (P3-1). |
| +2. The dispositions, "Stated and not changed", bullet 2 (`DISPOSITIONS:484`): "The buyer then waits on a person." No stage-one outcome is available to that person (P2-1). |
| +3. The dispositions' account of the found defect: the scenario described needs the provider to change a canceled subscription's period, and the test fakes exactly that. State that it is defensive. |
| + |
| +## To verify against Stripe test mode before enabling |
| + |
| +Every earlier list stands. Add: |
| +1. **Subscription mode with 3-D Secure abandoned:** does a Subscription exist before the session completes, and with what metadata? |
| +2. **A declined card, then a second card on the same session:** how many Subscriptions name the intent? |
| +3. **SEPA or Bacs:** the session's status and payment status at completion, and the subscription's status, through a failed debit. |
| +4. **Event order:** `customer.subscription.created` against `checkout.session.completed`, and whether the session can read `open` once its subscription exists. |
| +5. **A canceled subscription:** can anything but its metadata change? |
| +6. **Checkout Session metadata:** can it be edited after creation? |
| + |
| +**Verdict: accept with fixes.** Settle P2-1 by test-mode check 1–2 before enabling (fix it if Checkout makes subscriptions before completion; otherwise correct the claim), and fix P3-1 to P3-4 and the claims. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review23.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review23.md| new file mode 100644 |
| index 000000000..51fc0e456 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review23.md |
| @@ -0,0 +1,103 @@ |
| +**Verdict: accept with fixes.** Every round-twenty-two finding is closed or honestly stated, and each new test fails without its fix. I found no P1 or P2. What remains is three P3s and three claims to correct; none needs a code change before the round is accepted. On that basis I would count review eighteen's P2 as closed at `2618403f`. |
| + |
| +This was source-only, read with `git show 2618403f:`. I modified nothing and ran nothing. The delta (`05ccec07..2618403f`) touches no pricing, form, quote, cookie, allowance-trigger, export or site code, so last round's conclusions on A–K, L–ZZZ2 and the site at `01b62f7` stand. |
| + |
| +## Round twenty-two, finding by finding |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Astra 1 (P2)**: an account could be retired under an open checkout dispute or refusal | **Closed.** `checkBillingRetirement` (`billingstore.go:1504-1521`) now refuses on an open `checkout_refused` or `checkout_fate_disputed` review. It runs in both the reservation (`:1533`) and `DeleteAccount` (`ownership.go:731`). | **No.** In `TestAnAccountIsNotRetiredUnderAnOpenCheckoutDisputeOrRefusal` (`billingnamed_test.go:711`), the old query would reserve in both held cases. The test also has a control. | |
| +| **Astra 2 (P2)**: a session refusal filed after another console consumed the intent was lost | **Closed.** The gone-intent branch now files a dispute against `billing_consumed_intents` (`billingstore.go:1256-1270`). | **No.** Without the fix the filing classifies an empty subscription, `purchaseOfSessionTx` never searches consumed purchases, and the count is 0. The test drives `fileRefusal` directly, not two consoles, as the dispositions say. | |
| +| **Astra 3 (P2)**: a click could return a pay link after a review opened mid-read | **Closed.** `st.Open \|\| st.Disputed` is checked after the first read (`billingservice.go:653`), after the second (`:717`), and before a new link is disclosed (`:780-785`). An undisclosed new session is still expired by the pass: `pinned()` prefers `intent.SessionID` (`billingretirement.go:255`). | **No.** First read: without the check the click goes on to expire the session, and the test asserts no expiry was requested and gets the wrong error. Second read and a new session: the URL is returned. | |
| +| **Astra 4 (P3)**: a broken listing page dropped the repeat and what followed it | **Closed** (`catalog.go:164-182`). Entries with no usable id are skipped, since nothing can be recorded without an id. On error the wrapper still hands the caller nothing. | **No**, for both the client test and `TestABrokenListingStillRecordsBothAnswers`. | |
| +| **Opus P2-1**: the dead subscription | Answered as a gate and a decision; see the next section. | No test, correctly: nothing changed in code. | |
| +| **Opus P3-1**: the unstated cleanup exception | **Closed.** It is now named in the header (`billingretirement.go:91-95`) and in the design. | n/a | |
| +| **Opus P3-2**: a subscription classified under its metadata's account | **Closed.** `accountOfIntentTx` (`billingretirement.go:762-782`) finds the account by intent id: standing, then consumed, then retired, then a resolution. The resolution's `subscription` column holds the intent id for refusals, as at `:639`. `readSubscription` records before reconcile's `AccountBySqid` can return. | **No.** Under both "no account" and "zzzz" the old code returned nil, and nothing was attached. | |
| +| **Opus P3-3**: the pass reported a hold it caused as a failure | **Closed at both sites** (`billingpass.go:267`, `:297-301`). | **No** for the `refuseSession` site. The recovery site has no test (P3-3 below). | |
| +| **Opus P3-4**: two keys for a dispute about a consumed intent with no session | **Closed in the classifier** (`billingretirement.go:660-664`), but the new filing uses a third key (P3-1 below). | **No.** | |
| +| **Opus claims 1–3** | All corrected: the exception is named twice, "held with no disposition" is at `DISPOSITIONS:484`, and the found defect is described as defensive. | n/a | |
| + |
| +**Where the reviewers disagreed on severity, Astra's reading was right on all three P2s:** |
| +- **Astra 1** released a case with an active hand-made subscription at the provider into an account no door reaches. |
| +- **Astra 3** offered a payable session that could not record, which takes money into a hold. |
| +- **Astra 2** is a P2 under the owner's "nothing observed is dropped" rule, though not on money: the purchase recorded on terms the other console verified. |
| + |
| +**My P2-1:** Astra listed it as residual, and it is a conditional P2. It needs provider behaviour that is not yet verified. |
| + |
| +## Opus P2-1 as a gate rather than code |
| + |
| +This is honest and sufficient for stage one: |
| +- Billing is off. |
| +- `ENABLEMENT.md` §6 step 2 (on the launch branch) makes "does Checkout ever create a Subscription before its session completes" the first check of the rehearsal, before anything else. |
| +- It says that if the answer is yes, decision 15 is needed and code must be built. |
| +- Reading an attached subscription's status in code now would be wrong. |
| + |
| +**Why reading status now would be wrong.** That is exactly what Astra's item 8 forbids: `incomplete_expired` or `canceled` is not settlement from status alone. The release would have to rest on a fresh read of the subscription's invoices and payments. |
| + |
| +**A correction to decision 15's proposal.** It proposes "a fresh read of the subscription and of its invoices showing none paid". That is too weak against item 8, because an invoice that is `open`, or whose payment is still `processing` (SEPA or Bacs), can still collect. The predicate should be: every invoice `void` (or `draft`), no succeeded or processing charge or PaymentIntent, and the subscription `incomplete_expired`. Anything else stays held. |
| + |
| +## Findings |
| + |
| +### P3-1. The late-refusal dispute uses a third key for an intent consumed with no session |
| +- **Where:** `billingstore.go:1263-1265` sets `usedSession = sessionID`. The record and the classifier (`billingretirement.go:660-664`) use the intent id. |
| +- **Scenario:** an intent is consumed with no session; no path does this today. A late session refusal then opens a dispute keyed by the session, and a later subscription opens a second one keyed by the intent. That is the same split P3-4 closed. |
| +- **Fix:** use `intent.ID`. |
| + |
| +### P3-2. Evidence first seen after an account is deleted lands where no operator looks |
| +- **Where:** `accountOfIntentTx` and `classifySubscriptionTx` do not check `accounts.deleted_at`. |
| +- **Scenario:** |
| + 1. An account's purchase ended and the account was retired; the gate now passes only with no open checkout review. |
| + 2. A hand-made H naming the consumed intent is read afterwards. |
| + 3. A dispute is filed on the deleted account. Astra noted at `05ccec07` that the review listing excludes deleted accounts, and the resolution door answers 404. |
| + 4. Reconcile then answers a bare identity refusal. |
| +- **Effect:** the row is durable but invisible. This is the only remaining door by which seen evidence ends as nothing but a rejected event, beyond the two stated exceptions. |
| +- **Fix:** list reviews of deleted accounts in the operator listing, or state the limit beside the two exceptions. |
| + |
| +### P3-3. The recovery site's filter of the hold it caused has no test |
| +- **Where:** `billingpass.go:297-301`. |
| +- **Effect:** removing that filter survives `TestThePassDoesNotFailOnAHoldItCaused`, which reaches only `:267`. The dispositions' "all nine die" covers one site. |
| +- **Fix:** add a case where recovery's refusal leaves a hold. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **`billingservice.go:790-793`** says a stillborn intent "is discarded unconditionally". `discardUnreadyIntent` refuses under an open review or a named subscription. Astra raised this at `05ccec07` and it was not answered. |
| +2. **`billingretirement.go:965-966`** says a retried refusal is due again "an hour later at the earliest". The schedule (`:799`) is 60 s, 300 s, 900 s, then 3600 s. Also Astra's, also unanswered. |
| +3. **The dispositions' "Where" for Opus P2-1** cites `DECISIONS-FOR-PAUL.md`, which is not in `2618403f`. It is on the launch branch, as decision 15. |
| +4. **`ENABLEMENT.md` §6.2** says "if no, the rule costs an honest buyer nothing". That holds only if the provider also never names one intent from two subscriptions by any other means (test-mode items 5 and 6 below). Qualify it. |
| + |
| +## KKKK, LLLL and MMMM |
| + |
| +**KKKK, every door out of the protocol's sight:** |
| + |
| +| Door | Can seen evidence still end as nothing but a rejected event or a retired account? | |
| +|---|---| |
| +| Account retirement (reservation and `DeleteAccount`) | No longer while anything seen is open. After deletion, only through P3-2. | |
| +| The deletion's preparation | No: `discardUnreadyIntent` refuses under a review or an attachment. | |
| +| The operator's retire | No: it is held with anything attached; a later subscription is a dispute against the retirement archive. | |
| +| The record | No: the others seen become disputes; a late refusal is now one too. | |
| +| The click | No: it re-checks after each read and before disclosing a link. | |
| +| Recovery | No: partial and broken pages are recorded. | |
| +| The webhook's settled answers | No: each comes after `readSubscription` has recorded by intent. | |
| + |
| +The stated exceptions remain: an intent that expired in the ordinary way, and an unready intent cleaned up, each with nothing seen naming it. |
| + |
| +**LLLL, honest buyers.** Nothing in this delta is worse than at `b260ac8e` in any sequence the provider is known to produce: |
| +- The new retirement gate, the late-refusal dispute and the click's checks fire only on a review or a genuine disagreement. |
| +- A genuine disagreement needs a hand-made subscription, session metadata edited between reads, or two builds disagreeing during a roll. The roll rule covers the last. |
| + |
| +**MMMM.** Nothing in code is required before this round is accepted. The P3s and claims above are fixes to make, not gates. What remains is: |
| +- decision 15, with the corrected predicate; |
| +- the test-mode rehearsal and its gate; |
| +- the owner's approvals the dispositions list. |
| + |
| +## To verify against Stripe test mode before enabling |
| + |
| +Every earlier list stands. In order: |
| +1. **The gate:** in subscription mode, does Checkout create a Subscription before the session completes, and with what metadata? Run it with 3-D Secure abandoned, a declined card then another on the same session, and a delayed debit. |
| +2. **An abandoned `incomplete_expired` subscription:** what state are its invoices in, and does any charge or PaymentIntent exist? This is the input to decision 15. |
| +3. **SEPA or Bacs:** the session's status and payment status, and the subscription's, through a failed debit. |
| +4. **Event order:** `customer.subscription.created` against `checkout.session.completed`, and whether the session can read `open` once its subscription exists. |
| +5. **Metadata:** can a Checkout Session's or a Subscription's metadata be edited after creation, and by whom? |
| +6. **A canceled subscription:** can anything but its metadata change? |
| + |
| +**Verdict: accept with fixes** (P3-1 to P3-3 and the four claims; the gate and decision 15 before enabling). |
diff --git a/internal/billingstripe/catalog.go b/internal/billingstripe/catalog.go| index 6978681d8..159b68334 100644 |
| --- a/internal/billingstripe/catalog.go |
| +++ b/internal/billingstripe/catalog.go |
| @@ -150,8 +150,8 @@ func (c *Client) listCheckouts(ctx context.Context, customerID string, createdAf |
| q.Set("starting_after", after) |
| } |
| var page struct { |
| - Data []Session `json:"data"` |
| - HasMore bool `json:"has_more"` |
| + Data []json.RawMessage `json:"data"` |
| + HasMore bool `json:"has_more"` |
| } |
| // A listing that fails after its first page returns, with the |
| // error, the sessions of the pages it did read: each of those was |
| @@ -161,12 +161,36 @@ func (c *Client) listCheckouts(ctx context.Context, customerID string, createdAf |
| if err := c.do(ctx, http.MethodGet, "/v1/checkout/sessions", q, "", &page); err != nil { |
| return out, err |
| } |
| - for _, session := range page.Data { |
| - if !validID(session.ID) || seen[session.ID] { |
| - return out, &Error{Code: CodeBrokenPagination} |
| + // Each session on the page is decoded on its own: one that does |
| + // not decode fails the listing, and the ones that did decode are |
| + // still returned with that error. A page that repeats a session, |
| + // or carries one with no usable id, is broken pagination and the |
| + // listing is refused the same way, with what the page did say: a |
| + // session seen twice is two answers about it, and the second may |
| + // differ. |
| + var pageErr error |
| + lastID := "" |
| + for _, raw := range page.Data { |
| + var session Session |
| + if err := json.Unmarshal(raw, &session); err != nil { |
| + pageErr = &Error{Code: CodeDecodeError, HTTPStatus: http.StatusOK} |
| + continue |
| + } |
| + if !validID(session.ID) { |
| + if pageErr == nil { |
| + pageErr = &Error{Code: CodeBrokenPagination} |
| + } |
| + continue |
| + } |
| + if seen[session.ID] && pageErr == nil { |
| + pageErr = &Error{Code: CodeBrokenPagination} |
| } |
| seen[session.ID] = true |
| out = append(out, session) |
| + lastID = session.ID |
| + } |
| + if pageErr != nil { |
| + return out, pageErr |
| } |
| if len(out) > 10000 { |
| return out, &Error{Code: CodeTooManyRecords} |
| @@ -177,7 +201,7 @@ func (c *Client) listCheckouts(ctx context.Context, customerID string, createdAf |
| if len(page.Data) == 0 { |
| return out, &Error{Code: CodeBrokenPagination} |
| } |
| - after = page.Data[len(page.Data)-1].ID |
| + after = lastID |
| } |
| } |
diff --git a/internal/billingstripe/catalog_test.go b/internal/billingstripe/catalog_test.go| index 7c041ab07..c59efbeac 100644 |
| --- a/internal/billingstripe/catalog_test.go |
| +++ b/internal/billingstripe/catalog_test.go |
| @@ -3,6 +3,7 @@ package billingstripe |
| import ( |
| "context" |
| "encoding/json" |
| + "errors" |
| "fmt" |
| "net/http" |
| "net/http/httptest" |
| @@ -327,6 +328,55 @@ func TestListCheckouts_ALaterPagesFailureKeepsTheEarlierPages(t *testing.T) { |
| } |
| } |
| +// A page with a session that does not decode fails the listing, and the |
| +// sessions on it that did decode are still returned with the error. |
| +func TestListCheckouts_AMalformedSessionKeepsTheOthersOnItsPage(t *testing.T) { |
| + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + _ = json.NewEncoder(w).Encode(map[string]any{ |
| + "data": []map[string]any{{"id": "cs_1", "status": "complete", "livemode": true}, {"id": "cs_bad"}, {"id": "cs_2", "status": "open", "livemode": true}}, |
| + "has_more": false, |
| + }) |
| + })) |
| + defer srv.Close() |
| + c := testClient(t, srv.URL) |
| + out, err := c.ListCheckouts(context.Background(), 0, 100) |
| + var listErr *Error |
| + if !errors.As(err, &listErr) || listErr.Code != CodeDecodeError { |
| + t.Fatalf("a session that does not decode: %v; want a decode error", err) |
| + } |
| + if len(out) != 2 || out[0].ID != "cs_1" || out[0].Status != "complete" || out[1].ID != "cs_2" { |
| + t.Fatalf("the sessions that did decode: %+v; want the ones before and after the malformed one", out) |
| + } |
| +} |
| + |
| +// A page that repeats a session is broken pagination, and what it said is |
| +// still returned with the error: the repeat, and what follows it. |
| +func TestListCheckouts_ABrokenPageStillReturnsWhatItSaid(t *testing.T) { |
| + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + if r.URL.Query().Get("starting_after") == "" { |
| + _ = json.NewEncoder(w).Encode(map[string]any{ |
| + "data": []map[string]any{{"id": "cs_1", "status": "expired", "livemode": true}}, |
| + "has_more": true, |
| + }) |
| + return |
| + } |
| + _ = json.NewEncoder(w).Encode(map[string]any{ |
| + "data": []map[string]any{{"id": "cs_1", "status": "complete", "livemode": true}, {"id": "cs_2", "status": "open", "livemode": true}}, |
| + "has_more": false, |
| + }) |
| + })) |
| + defer srv.Close() |
| + c := testClient(t, srv.URL) |
| + out, err := c.ListCheckouts(context.Background(), 0, 100) |
| + var listErr *Error |
| + if !errors.As(err, &listErr) || listErr.Code != CodeBrokenPagination { |
| + t.Fatalf("a repeated session: %v; want broken pagination", err) |
| + } |
| + if len(out) != 3 || out[0].ID != "cs_1" || out[0].Status != "expired" || out[1].ID != "cs_1" || out[1].Status != "complete" || out[2].ID != "cs_2" { |
| + t.Fatalf("what the pages said: %+v; want both answers about cs_1 and the session after the repeat", out) |
| + } |
| +} |
| + |
| func TestListCheckouts_RejectsInvalidBoundsWithoutHTTPCall(t *testing.T) { |
| calls := 0 |
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { calls++ })) |
diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go| index e8f4d54c2..c35475960 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -3500,12 +3500,19 @@ func TestABoundSessionReadBackAsNobodysIsReviewed(t *testing.T) { |
| if standing, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || standing.ID != intent.ID { |
| t.Fatalf("intent %+v %v; want it standing", standing, err) |
| } |
| - // The operator cannot retire it: a session that reads back as another |
| - // account's proves nothing about itself, so its fate is unproven and |
| - // the intent is held; and the buyer's next click is offered nothing. |
| + // The operator cannot retire it. The subscription that named the |
| + // intent (under the other account's name) is recorded against the |
| + // intent and holds the retire first; and a session that reads back as |
| + // another account's proves nothing about itself, so with that taken |
| + // away by hand the fate is unproven and the intent is held for that. |
| + // The buyer's next click is offered nothing. |
| if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil { |
| t.Fatal(err) |
| } |
| + heldRetire(t, fx, acct, intent.ID, "paul@keymail.dev", "sub_paid", f.now.Add(10*time.Minute)) |
| + if _, err := fx.st.db.Exec(`DELETE FROM billing_review_obligations WHERE account_id=? AND intent_id=?`, acct.ID, intent.ID); err != nil { |
| + t.Fatal(err) |
| + } |
| heldRetire(t, fx, acct, intent.ID, "paul@keymail.dev", "not yet proven expired", f.now.Add(10*time.Minute)) |
| if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now.Add(20*time.Minute)); dest != "" || !errors.Is(err, errBillingUnderReview) { |
| t.Fatalf("a click under the review: dest %q err %v; want nothing offered", dest, err) |
diff --git a/internal/console/billingnamed_test.go b/internal/console/billingnamed_test.go| index 3e21e25a0..4707e90ac 100644 |
| --- a/internal/console/billingnamed_test.go |
| +++ b/internal/console/billingnamed_test.go |
| @@ -673,3 +673,507 @@ func TestTheRefusalsTurnTakesTheLongestDueFirst(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// Review twenty-two's counterexamples. |
| + |
| +// purchased opens a GBP purchase and records it; with handMade, a |
| +// subscription made by hand was seen naming the intent first, so the |
| +// record leaves a dispute. |
| +func purchased(t *testing.T, handMade bool) (*consoleFixture, *fakeBillingStripe, *Account, billingIntent) { |
| + 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) |
| + f.mu.Lock() |
| + f.subscriptionPrice = intent.Price |
| + f.otherSubscriptions = map[string]bool{"sub_hand": true} |
| + f.mu.Unlock() |
| + if handMade { |
| + _ = f.webhookPost(t, fx, "evt_hand_first", "customer.subscription.created", "sub_hand") |
| + } |
| + f.mu.Lock() |
| + f.sessionStatus = "complete" |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_own", "customer.subscription.created", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the purchase: %d", code) |
| + } |
| + if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "sub_paid" { |
| + t.Fatalf("fixture: %+v", state) |
| + } |
| + return fx, f, acct, intent |
| +} |
| + |
| +// An account is not retired while a checkout dispute, or a checkout |
| +// refusal, is open on it. (Review twenty-two, Astra 1.) |
| +func TestAnAccountIsNotRetiredUnderAnOpenCheckoutDisputeOrRefusal(t *testing.T) { |
| + ended := func(t *testing.T, fx *consoleFixture, f *fakeBillingStripe) { |
| + t.Helper() |
| + f.mu.Lock() |
| + f.subscriptionStatus = "canceled" |
| + f.mu.Unlock() |
| + if err := fx.srv.reconcileBillingSubscription(context.Background(), "sub_paid", "", f.now.Add(time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + // The control: an honest purchase that has ended does not stop it. |
| + fx, f, acct, _ := purchased(t, false) |
| + ended(t, fx, f) |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatalf("an account whose only purchase has ended: %v; want it retirable", err) |
| + } |
| + // With a subscription made by hand outstanding as a dispute, it is |
| + // not. |
| + fx, f, acct, intent := purchased(t, true) |
| + if n := disputesOf(t, fx, acct, intent.SessionID, "%"); n != 1 { |
| + t.Fatalf("fixture: %d disputes", n) |
| + } |
| + ended(t, fx, f) |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); !errors.Is(err, errBillingRetirement) { |
| + t.Fatalf("an account under an open checkout dispute: %v; want the retirement refused", err) |
| + } |
| + // Nor under an open refusal whose intent is gone (by hand here: no |
| + // path removes an intent under its review). |
| + fx2, f2, acct2, intent2 := unreadyIntent(t, func(billingIntent) string { return currencyPriceID(plans.PlanPersonal, plans.BandD, plans.GBP) }) |
| + _ = f2.webhookPost(t, fx2, "evt_hand_retirement", "customer.subscription.created", "sub_paid") |
| + for _, q := range []string{`DROP TRIGGER billing_refused_intent_delete_barrier`, `DELETE FROM billing_checkout_intents WHERE account_id=` + fmt.Sprint(acct2.ID)} { |
| + if _, err := fx2.st.db.Exec(q); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if open, _ := fx2.st.refusalReviewOpen(acct2.ID, intent2.ID); !open { |
| + t.Fatal("fixture: no open refusal") |
| + } |
| + if err := fx2.st.reserveBillingRetirement(acct2.ID, f2.now.Add(time.Hour)); !errors.Is(err, errBillingRetirement) { |
| + t.Fatalf("an account under an open checkout refusal: %v; want the retirement refused", err) |
| + } |
| +} |
| + |
| +// A console that judged a session not the purchase, and files its |
| +// refusal after another console recorded the purchase, does not lose the |
| +// refusal: it is kept against what the record consumed. (Review |
| +// twenty-two, Astra 2.) |
| +func TestARefusalOfASessionFiledAfterItsPurchaseRecordedIsKept(t *testing.T) { |
| + fx, f, acct, intent := purchased(t, false) |
| + if n := disputesOf(t, fx, acct, intent.SessionID, "%"); n != 0 { |
| + t.Fatalf("fixture: %d disputes", n) |
| + } |
| + // This console read the session (as it then was) before the record, |
| + // and files now: through the server's own filing, with the session |
| + // as it read it. |
| + session, err := fx.srv.readCheckout(context.Background(), intent.SessionID) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + for i := 0; i < 2; i++ { |
| + if err := fx.srv.fileRefusal(intent, intent.SessionID, "", session, f.now.Add(time.Minute)); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if n := disputesOf(t, fx, acct, intent.SessionID, "a refusal of session "+intent.SessionID+" was filed after the purchase recorded with sub_paid"); n != 1 { |
| + t.Fatalf("%d disputes for the late refusal; want one", n) |
| + } |
| + var reviews int |
| + _ = fx.st.db.QueryRow(`SELECT count(*) FROM billing_reviews WHERE account_id=? AND reason=? AND resolved_at=0`, acct.ID, billingFateDisputedReason).Scan(&reviews) |
| + if reviews != 1 { |
| + t.Fatalf("%d open dispute reviews; want the account held", reviews) |
| + } |
| + if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "sub_paid" { |
| + t.Fatalf("the recorded purchase changed: %+v", state) |
| + } |
| +} |
| + |
| +// A click offers no link for an intent that came under review while the |
| +// click was reading the provider: after its first read, after its |
| +// second, and after it made a new session. (Review twenty-two, Astra 3.) |
| +func TestAClickOffersNothingWhenAReviewOpensWhileItReads(t *testing.T) { |
| + open := func(t *testing.T) (*consoleFixture, *fakeBillingStripe, *Account, billingIntent) { |
| + 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) |
| + // The other console's expiry of the session does not go through: |
| + // the session stays open. |
| + f.mu.Lock() |
| + f.failExpire = 100 |
| + f.mu.Unlock() |
| + return fx, f, acct, intent |
| + } |
| + // The first read's case is a click that changed its mind (another |
| + // country): without the check after that read, the click would go on |
| + // to expire the session under its review and fail on that; the second |
| + // read's case is the same click again. |
| + for name, at := range map[string]int{"the first read": 0, "the second read": 1} { |
| + fx, f, acct, intent := open(t) |
| + country, postal := "GB", "" |
| + if at == 0 { |
| + country, postal = "US", "94107" |
| + } |
| + answers := []func(*fakeBillingStripe){func(*fakeBillingStripe) {}, func(*fakeBillingStripe) {}} |
| + answers[at] = func(*fakeBillingStripe) { |
| + // Another console files a refusal of this intent, with no |
| + // dispute, while this click's read is in flight. |
| + if err := fx.st.fileCheckoutRefusal(intent, intent.SessionID, "", f.now); err != nil { |
| + t.Error(err) |
| + } |
| + } |
| + f.mu.Lock() |
| + f.sessionAnswers = answers |
| + f.mu.Unlock() |
| + dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, country, postal, "gbp", f.now.Add(time.Minute)) |
| + if dest != "" || !errors.Is(err, errBillingPending) { |
| + t.Fatalf("a review opened during %s: dest %q err %v; want no link, under review", name, dest, err) |
| + } |
| + if f.expireCalls != 0 { |
| + t.Fatalf("%s: the click requested the session's expiry %d time(s) under the review", name, f.expireCalls) |
| + } |
| + if st, _ := fx.st.refusalState(acct.ID, intent.ID); !st.Open || st.Disputed != 0 { |
| + t.Fatalf("%s: state %+v; want the review open and no dispute", name, st) |
| + } |
| + } |
| + // A new session: the refusal is filed while the provider is making it |
| + // (a subscription that names the new intent, seen by another |
| + // console). The session is bound and its link is not disclosed. |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + f.mu.Lock() |
| + f.sessionAnswers = []func(*fakeBillingStripe){func(*fakeBillingStripe) { |
| + intent, err := fx.st.billingCheckoutIntent(acct.ID) |
| + if err != nil { |
| + t.Error(err) |
| + return |
| + } |
| + if err := fx.st.fileCheckoutRefusal(intent, "", "sub_hand", f.now); err != nil { |
| + t.Error(err) |
| + } |
| + }} |
| + f.mu.Unlock() |
| + dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now) |
| + if dest != "" || !errors.Is(err, errBillingPending) { |
| + t.Fatalf("a review opened while the session was made: dest %q err %v; want no link, under review", dest, err) |
| + } |
| + if intent, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || intent.SessionID == "" { |
| + t.Fatalf("the intent: %+v %v; want it bound to the session that was made, for the pass to expire", intent, err) |
| + } |
| +} |
| + |
| +// A listing whose second page repeats a session, reading differently, |
| +// is refused as broken, and both answers are recorded. (Review |
| +// twenty-two, Astra 4.) |
| +func TestABrokenListingStillRecordsBothAnswers(t *testing.T) { |
| + fx, f, acct, intent, _ := retiredOnAProvenExpiry(t, "code-billing-op-listing-repeat") |
| + f.mu.Lock() |
| + f.listRepeatsOnSecondPage = true |
| + // The first page reads the session expired, as it was archived; the |
| + // second reads it complete. |
| + f.sessionAnswers = []func(*fakeBillingStripe){func(*fakeBillingStripe) {}, func(f *fakeBillingStripe) { f.sessionStatus = "complete" }} |
| + f.mu.Unlock() |
| + sessions, err := fx.srv.listCheckouts(context.Background(), "cus_paid", f.now.Add(-time.Hour).Unix(), f.now.Add(time.Hour).Unix()) |
| + if err == nil || sessions != nil { |
| + t.Fatalf("the listing: %v %v; want it refused, and no sessions handed on", sessions, err) |
| + } |
| + if n := disputesOf(t, fx, acct, intent.SessionID, "read back complete after its intent was retired on a proven expiry"); n != 1 { |
| + t.Fatalf("%d disputes from the repeated session's second answer; want one", n) |
| + } |
| +} |
| + |
| +// A subscription is recorded against the intent it names under the |
| +// account the console recorded that intent for, whatever account its own |
| +// metadata names, or none. (Review twenty-two, Opus P3-2.) |
| +func TestASubscriptionIsRecordedUnderTheIntentsOwnAccount(t *testing.T) { |
| + for name, sqid := range map[string]string{"no account": "", "another account": "zzzz"} { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + intent, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + f.mu.Lock() |
| + f.meta["account_sqid"] = sqid |
| + f.subscriptionPrice = intent.Price |
| + f.mu.Unlock() |
| + _ = f.webhookPost(t, fx, "evt_hand_other_account", "customer.subscription.created", "sub_paid") |
| + st, err := fx.st.refusalState(acct.ID, intent.ID) |
| + if err != nil || len(st.Obligations) != 1 || st.Obligations[0] != "sub_paid" { |
| + t.Fatalf("a subscription naming the intent and %s: state %+v %v; want it recorded against the intent", name, st, err) |
| + } |
| + } |
| +} |
| + |
| +// The pass does not report as its own failure an intent it left under |
| +// review: an open session that is no longer the purchase is expired, and |
| +// because a subscription names the intent the expiry files the refusal |
| +// instead of retiring it. (Review twenty-two, Opus P3-3.) |
| +func TestThePassDoesNotFailOnAHoldItCaused(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + intent, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + f.mu.Lock() |
| + f.subscriptionPrice = intent.Price |
| + f.mu.Unlock() |
| + _ = f.webhookPost(t, fx, "evt_hand_pass", "customer.subscription.created", "sub_paid") |
| + // The open session now reads in another currency than the intent's. |
| + f.mu.Lock() |
| + f.sessionCurrency = "eur" |
| + f.mu.Unlock() |
| + if err := fx.srv.billingPass(context.Background(), f.now.Add(time.Minute)); err != nil { |
| + t.Fatalf("the pass: %v; want no failure", err) |
| + } |
| + standing(t, fx, acct, intent) |
| + if st, _ := fx.st.refusalState(acct.ID, intent.ID); !st.Open || len(st.Obligations) != 1 || f.sessionStatus != "expired" { |
| + t.Fatalf("state %+v, session %s; want the session expired and the intent under review with the subscription attached", st, f.sessionStatus) |
| + } |
| +} |
| + |
| +// The record and the classification use one key for a dispute about an |
| +// intent that was consumed with no session (no path consumes one today). |
| +func TestADisputeAboutAConsumedIntentWithNoSessionIsKeyedByTheIntent(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + if _, err := fx.st.db.Exec(`INSERT INTO billing_consumed_intents(account_id,intent_id,session_id,subscription,customer,consumed_at) VALUES(?,?,?,?,?,?)`, acct.ID, "ci_no_session", "", "sub_first", "cus_paid", f.now.Unix()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.noteSubscriptionSeen("", "ci_no_session", "sub_other", f.now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n := disputesOf(t, fx, acct, "ci_no_session", "subscription sub_other names the intent whose purchase recorded with sub_first"); n != 1 { |
| + t.Fatalf("%d disputes keyed by the intent; want one", n) |
| + } |
| + if err := fx.st.noteSubscriptionSeen("", "ci_no_session", "sub_first", f.now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n := disputesOf(t, fx, acct, "ci_no_session", "%"); n != 1 { |
| + t.Fatalf("%d disputes after the purchase's own subscription was seen", n) |
| + } |
| +} |
| + |
| +// Review twenty-three's counterexamples. |
| + |
| +// An account's retirement has a point of no return in the database, before |
| +// anything outside it is written. Checkout evidence that lands before it |
| +// withdraws the pending retirement, and the commit then refuses; evidence |
| +// that lands after it stays with the account, which is deleted, and is |
| +// listed and resolvable under the deleted account. (Review twenty-three, |
| +// Astra 1; Opus P3-2.) |
| +func TestARetirementIsCommittedInTheDatabaseAndEvidenceSerialisesAgainstIt(t *testing.T) { |
| + // On an account that can be deleted (the fixture's own is the ops |
| + // account): a second account, with the fixture's member, that buys |
| + // and whose purchase ends. |
| + ended := func(t *testing.T) (*consoleFixture, *fakeBillingStripe, *Account, billingIntent) { |
| + t.Helper() |
| + fx, f, ops := newCurrencyFixture(t) |
| + if err := fx.st.CreateAccount(ops.ID+100, "cdef", "Other", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.AddMember(ops.ID+100, "paul@keymail.dev", RoleOwner, time.Now()); err != nil && !strings.Contains(err.Error(), "already") { |
| + t.Fatal(err) |
| + } |
| + acct, _ := fx.st.AccountBySqid("cdef") |
| + f.customerCountry = "GB" |
| + f.mu.Lock() |
| + f.sqid = "cdef" |
| + f.mu.Unlock() |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + intent, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + f.mu.Lock() |
| + f.sessionStatus, f.subscriptionPrice = "complete", intent.Price |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_own_other", "customer.subscription.created", "sub_paid"); code != http.StatusNoContent { |
| + t.Fatalf("the purchase: %d", code) |
| + } |
| + f.mu.Lock() |
| + f.subscriptionStatus = "canceled" |
| + f.mu.Unlock() |
| + if err := fx.srv.reconcileBillingSubscription(context.Background(), "sub_paid", "", f.now.Add(time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "sub_paid" || state.Status != "canceled" { |
| + t.Fatalf("fixture: %+v", state) |
| + } |
| + return fx, f, acct, intent |
| + } |
| + reserved := func(t *testing.T, fx *consoleFixture, acct *Account) (reserved, committed bool) { |
| + t.Helper() |
| + var at int64 |
| + err := fx.st.db.QueryRow(`SELECT committed_at FROM billing_retirements WHERE account_id=?`, acct.ID).Scan(&at) |
| + if errors.Is(err, sql.ErrNoRows) { |
| + return false, false |
| + } |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return true, at != 0 |
| + } |
| + t.Run("evidence before the commit withdraws the retirement", func(t *testing.T) { |
| + fx, f, acct, _ := ended(t) |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + // Another console reads a subscription made by hand naming the |
| + // consumed intent, through the production classification. |
| + f.mu.Lock() |
| + f.otherSubscriptions = map[string]bool{"sub_hand": true} |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_hand_while_reserved", "customer.subscription.created", "sub_hand"); code != http.StatusNoContent { |
| + t.Fatalf("the hand-made subscription: %d", code) |
| + } |
| + if r, _ := reserved(t, fx, acct); r { |
| + t.Fatal("the pending retirement was not withdrawn by the evidence") |
| + } |
| + if err := fx.st.commitBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); !errors.Is(err, errBillingRetirement) { |
| + t.Fatalf("the commit after the evidence: %v; want it refused", err) |
| + } |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(3*time.Hour)); !errors.Is(err, errBillingRetirement) { |
| + t.Fatalf("a new reservation under the open dispute: %v; want it refused", err) |
| + } |
| + if err := fx.st.DeleteAccount(acct.ID, f.now.Add(3*time.Hour)); !errors.Is(err, errBillingRetirement) { |
| + t.Fatalf("the deletion under the open dispute: %v; want it refused", err) |
| + } |
| + }) |
| + t.Run("the commit re-reads every condition and needs a reservation", func(t *testing.T) { |
| + fx, f, acct, intent := ended(t) |
| + // Never reserved: nothing to commit. |
| + if err := fx.st.commitBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); !errors.Is(err, errBillingRetirement) { |
| + t.Fatalf("a commit with no reservation: %v; want it refused", err) |
| + } |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + // A review written without withdrawing the reservation (as an |
| + // older binary would write it): the commit reads it and refuses. |
| + if _, err := fx.st.db.Exec(`INSERT INTO billing_reviews(account_id,subscription,reason,created_at,last_seen_at,occurrences,protocol) VALUES(?,?,?,?,?,1,1)`, acct.ID, intent.SessionID, billingFateDisputedReason, f.now.Unix(), f.now.Unix()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if r, _ := reserved(t, fx, acct); !r { |
| + t.Fatal("fixture: the reservation went") |
| + } |
| + if err := fx.st.commitBillingRetirement(acct.ID, f.now.Add(3*time.Hour)); !errors.Is(err, errBillingRetirement) { |
| + t.Fatalf("the commit over an open review: %v; want it refused", err) |
| + } |
| + if _, c := reserved(t, fx, acct); c { |
| + t.Fatal("the retirement was committed over an open review") |
| + } |
| + }) |
| + t.Run("evidence after the commit stays with the deleted account", func(t *testing.T) { |
| + fx, f, acct, intent := ended(t) |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.commitBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if r, c := reserved(t, fx, acct); !r || !c { |
| + t.Fatalf("after the commit: reserved %v committed %v", r, c) |
| + } |
| + f.mu.Lock() |
| + f.otherSubscriptions = map[string]bool{"sub_hand": true} |
| + f.mu.Unlock() |
| + if code := f.webhookPost(t, fx, "evt_hand_after_commit", "customer.subscription.created", "sub_hand"); code != http.StatusNoContent { |
| + t.Fatalf("the hand-made subscription: %d", code) |
| + } |
| + if n := disputesOf(t, fx, acct, intent.SessionID, "%"); n != 1 { |
| + t.Fatalf("%d disputes; want the evidence recorded", n) |
| + } |
| + if r, c := reserved(t, fx, acct); !r || !c { |
| + t.Fatalf("after the evidence: reserved %v committed %v; want the committed retirement kept", r, c) |
| + } |
| + // The commit again (a retry after an ambiguous failure outside) |
| + // passes, and so does the deletion. |
| + if err := fx.st.commitBillingRetirement(acct.ID, f.now.Add(3*time.Hour)); err != nil { |
| + t.Fatalf("the commit again: %v", err) |
| + } |
| + if err := fx.st.DeleteAccount(acct.ID, f.now.Add(3*time.Hour)); err != nil { |
| + t.Fatalf("the deletion after the commit: %v", err) |
| + } |
| + if a, _ := fx.st.AccountByID(acct.ID); a != nil { |
| + t.Fatal("the account was not deleted") |
| + } |
| + // The operator still sees the review, under the deleted account, |
| + // and the door finds it (and holds it, as every dispute). |
| + token := billingOperatorToken(t, fx, "ops@keymail.dev", "code-billing-op-deleted") |
| + status, listing := getJSON(t, fx.ts.URL+"/api/billing/reviews", token) |
| + rows, _ := listing["reviews"].([]any) |
| + found := false |
| + for _, r := range rows { |
| + row, _ := r.(map[string]any) |
| + if row["reason"] == billingFateDisputedReason && row["subscription"] == intent.SessionID { |
| + found = true |
| + if row["account"] != acct.Sqid || row["account_deleted"] != true { |
| + t.Fatalf("the deleted account's review as listed: %v", row) |
| + } |
| + } |
| + } |
| + if status != http.StatusOK || !found { |
| + t.Fatalf("the deleted account's dispute is not listed: %d %v", status, listing) |
| + } |
| + body := map[string]any{"account": acct.Sqid, "subscription": intent.SessionID, "reason": billingFateDisputedReason, "occurrences": 1, "note": "looked"} |
| + if status, answer := postJSON(t, fx.ts.URL+"/api/billing/reviews/resolve", body, token); status != http.StatusConflict || !strings.Contains(fmt.Sprint(answer["error"]), "held") { |
| + t.Fatalf("the door for the deleted account's dispute: %d %v; want it found and held, not 404", status, answer) |
| + } |
| + }) |
| + t.Run("a review of another kind withdraws nothing", func(t *testing.T) { |
| + fx, f, acct, _ := ended(t) |
| + if err := fx.st.reserveBillingRetirement(acct.ID, f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + tx, _ := fx.st.db.Begin() |
| + if err := fileBillingReviewTx(tx, acct.ID, "sub_paid", "billing_location_changed", f.now.Add(2*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := tx.Commit(); err != nil { |
| + t.Fatal(err) |
| + } |
| + if r, _ := reserved(t, fx, acct); !r { |
| + t.Fatal("a review that does not stop a retirement withdrew the reservation") |
| + } |
| + }) |
| +} |
| + |
| +// The recovery of a lost session that refuses it, and leaves the intent |
| +// under review because a subscription names it, is not the pass's failure |
| +// either. (Review twenty-three, Opus P3-3.) |
| +func TestThePassDoesNotFailOnAHoldItsRecoveryCaused(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + // The create's answer is lost: the intent is ready and unbound. |
| + f.mu.Lock() |
| + f.failCreate = true |
| + f.mu.Unlock() |
| + if _, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err == nil { |
| + t.Fatal("fixture: the checkout went through") |
| + } |
| + intent, err := fx.st.billingCheckoutIntent(acct.ID) |
| + if err != nil || intent.SessionID != "" || !intent.CustomerReady { |
| + t.Fatalf("fixture: %+v %v; want a ready, unbound intent", intent, err) |
| + } |
| + // A subscription names it (on terms: a retry, and recorded). |
| + f.mu.Lock() |
| + f.subscriptionPrice = intent.Price |
| + f.mu.Unlock() |
| + _ = f.webhookPost(t, fx, "evt_hand_unbound_pass", "customer.subscription.created", "sub_paid") |
| + if st, _ := fx.st.refusalState(acct.ID, intent.ID); len(st.Obligations) != 1 { |
| + t.Fatalf("fixture: %+v", st) |
| + } |
| + // The pass recovers the session by listing; it reads in another |
| + // currency than the intent's, so it is refused and expired, and the |
| + // expiry files the refusal instead of retiring the intent. |
| + f.mu.Lock() |
| + f.sessionCurrency = "eur" |
| + f.mu.Unlock() |
| + if err := fx.srv.billingPass(context.Background(), f.now.Add(time.Minute)); err != nil { |
| + t.Fatalf("the pass: %v; want no failure", err) |
| + } |
| + standing(t, fx, acct, intent) |
| + if st, _ := fx.st.refusalState(acct.ID, intent.ID); !st.Open || st.Reference == "" || f.sessionStatus != "expired" { |
| + t.Fatalf("state %+v, session %s; want the refusal filed naming the recovered session, which was expired", st, f.sessionStatus) |
| + } |
| +} |
diff --git a/internal/console/billingoperator.go b/internal/console/billingoperator.go| index 91cf42b07..58c509561 100644 |
| --- a/internal/console/billingoperator.go |
| +++ b/internal/console/billingoperator.go |
| @@ -182,6 +182,9 @@ type billingReviewRow struct { |
| LastSeenAt int64 `json:"last_seen_at"` |
| // Reference: the refused session's id for a currency refusal, else "". |
| Reference string `json:"reference,omitempty"` |
| + // AccountDeleted: the review is a deleted account's (evidence that |
| + // landed after its retirement was committed). Still a person's to see. |
| + AccountDeleted bool `json:"account_deleted,omitempty"` |
| // Disposition, for a checkout refusal: the session's recorded fate, |
| // what is attached, and why retire or keep is held (empty: available). |
| Disposition *refusalDisposition `json:"disposition,omitempty"` |
| @@ -199,11 +202,11 @@ func (s *Server) handleAPIBillingReviews(w http.ResponseWriter, r *http.Request) |
| } |
| out := make([]billingReviewRow, 0, len(reviews)) |
| for _, x := range reviews { |
| - sqid := "" |
| - if acct, err := s.store.AccountByID(x.AccountID); err == nil && acct != nil { |
| - sqid = acct.Sqid |
| + sqid, deleted := "", false |
| + if acct, gone, err := s.store.accountEvenIfDeleted(x.AccountID); err == nil && acct != nil { |
| + sqid, deleted = acct.Sqid, gone |
| } |
| - row := billingReviewRow{Account: sqid, Subscription: x.Subscription, Reason: x.Reason, Occurrences: x.Occurrences, CreatedAt: x.CreatedAt, LastSeenAt: x.LastSeenAt, Reference: x.Reference} |
| + row := billingReviewRow{Account: sqid, Subscription: x.Subscription, Reason: x.Reason, Occurrences: x.Occurrences, CreatedAt: x.CreatedAt, LastSeenAt: x.LastSeenAt, Reference: x.Reference, AccountDeleted: deleted} |
| if x.Reason == billingCheckoutRefusedReason { |
| if st, err := s.store.refusalState(x.AccountID, x.Subscription); err == nil { |
| row.Disposition = st.disposition() |
| @@ -337,7 +340,7 @@ func (s *Server) handleAPIBillingReviewResolve(w http.ResponseWriter, r *http.Re |
| jsonError(w, http.StatusBadRequest, "outcome must be retire or keep") |
| return |
| } |
| - acct, err := s.store.AccountBySqid(req.Account) |
| + acct, _, err := s.store.accountBySqidEvenIfDeleted(req.Account) |
| if err != nil || acct == nil { |
| jsonError(w, http.StatusNotFound, "no such billing review") |
| return |
diff --git a/internal/console/billingpass.go b/internal/console/billingpass.go| index 521c3b224..08aac8958 100644 |
| --- a/internal/console/billingpass.go |
| +++ b/internal/console/billingpass.go |
| @@ -264,7 +264,7 @@ func (s *Server) reconcilePendingIntent(ctx context.Context, intent billingInten |
| return err |
| } |
| if session.ID == intent.SessionID && !b.intentSession(session, intent, acct) && b.ownSession(session, intent, acct) { |
| - if err := s.refuseSession(ctx, intent, session.ID, session, now); err != nil && !errors.Is(err, errBillingIdentity) { |
| + if err := s.refuseSession(ctx, intent, session.ID, session, now); err != nil && !errors.Is(err, errBillingIdentity) && !errors.Is(err, errBillingUnderReview) { |
| return err |
| } |
| } |
| @@ -294,6 +294,11 @@ func (s *Server) reconcilePendingIntent(ctx context.Context, intent billingInten |
| return nil |
| } |
| session, err := s.recoverReadyIntentSession(ctx, acct, intent, now, false) |
| + if errors.Is(err, errBillingUnderReview) { |
| + // The recovery refused a session and the store left the intent |
| + // under review: not a failure of the pass. |
| + return nil |
| + } |
| if err != nil { |
| return err |
| } |
diff --git a/internal/console/billingretirement.go b/internal/console/billingretirement.go| index 92d447854..3ff6aa1f2 100644 |
| --- a/internal/console/billingretirement.go |
| +++ b/internal/console/billingretirement.go |
| @@ -88,10 +88,23 @@ import ( |
| // - A read without identity, or of a session still open, before any |
| // fate is recorded: nothing is recorded for it to contradict, and |
| // with no fate both outcomes are held. |
| -// - A subscription that names an intent which expired in the ordinary |
| -// way with nothing seen naming it: nothing is kept of such an intent, |
| -// and the subscription is a rejected event with its account, in the |
| -// operator's list, and no hold. |
| +// - A subscription that names an intent which went with nothing seen |
| +// naming it and no purchase: one that expired in the ordinary way, |
| +// or one that never had a session and was cleaned up. Nothing is |
| +// kept of such an intent, and the subscription is a rejected event |
| +// with its account, in the operator's list, and no hold. |
| +// |
| +// What the rule costs. A subscription seen naming an intent stops that |
| +// intent's expiry and cleanup whatever the subscription's own state, a |
| +// dead one included, and files a refusal that stage one can only hold |
| +// (retire is held while anything is attached; keep needs a completed |
| +// session); a second subscription at a purchase's record is a dispute, |
| +// which stage one cannot resolve. If the provider ever makes a |
| +// subscription before its checkout session completes (an abandoned |
| +// authentication, a declined card retried), an honest buyer meets this |
| +// and is held with no disposition until what settles such a case is |
| +// decided. Whether it does is a check to make in the provider's test |
| +// mode before billing is enabled, not something this code knows. |
| const ( |
| sessionFateExpired = "expired" |
| @@ -156,6 +169,11 @@ func (s *Store) migrateBillingRetirement() error { |
| // another customer's is seen as the contradiction it is. |
| {"billing_retired_intents", "customer", `TEXT NOT NULL DEFAULT ''`}, |
| {"billing_retired_intents", "session_fate", `TEXT NOT NULL DEFAULT ''`}, |
| + // An account's retirement is committed in the database before |
| + // anything outside it is written (commitBillingRetirement): until |
| + // then checkout evidence cancels it, and from then on evidence |
| + // lands on an account that is being retired and stays visible. |
| + {"billing_retirements", "committed_at", `INTEGER NOT NULL DEFAULT 0`}, |
| } { |
| if err := s.addColumnIfMissing(c[0], c[1], c[2]); err != nil { |
| return err |
| @@ -638,12 +656,17 @@ func classifySubscriptionTx(tx *sql.Tx, accountID int64, intentID, subscription |
| // names it but the one it recorded with is a dispute. |
| var usedSession, usedSubscription string |
| err = tx.QueryRow(`SELECT session_id,subscription FROM billing_consumed_intents WHERE account_id=? AND intent_id=?`, accountID, intentID).Scan(&usedSession, &usedSubscription) |
| - if errors.Is(err, sql.ErrNoRows) || (err == nil && (usedSession == "" || subscription == usedSubscription)) { |
| + if errors.Is(err, sql.ErrNoRows) || (err == nil && subscription == usedSubscription) { |
| return nil |
| } |
| if err != nil { |
| return err |
| } |
| + if usedSession == "" { |
| + // The same key the record uses for an intent consumed with |
| + // no session (no path consumes one today). |
| + usedSession = intentID |
| + } |
| return disputeSessionTx(tx, accountID, usedSession, intentID, "subscription "+subscription+" names the intent whose purchase recorded with "+usedSubscription, now) |
| } |
| return err |
| @@ -701,7 +724,7 @@ func sessionBelongsElsewhereTx(tx *sql.Tx, sessionID string, accountID int64, in |
| // the provider, by the account and intent it names, in one transaction. |
| // The account's own recorded subscription is not evidence about anything. |
| func (s *Store) noteSubscriptionSeen(sqid, intentID, subscription string, now time.Time) error { |
| - if sqid == "" || intentID == "" || subscription == "" { |
| + if intentID == "" || subscription == "" { |
| return nil |
| } |
| tx, err := s.db.Begin() |
| @@ -709,13 +732,25 @@ func (s *Store) noteSubscriptionSeen(sqid, intentID, subscription string, now ti |
| return err |
| } |
| defer tx.Rollback() |
| - var accountID int64 |
| - if err := tx.QueryRow(`SELECT id FROM accounts WHERE sqid=?`, sqid).Scan(&accountID); err != nil { |
| - if errors.Is(err, sql.ErrNoRows) { |
| - return nil |
| - } |
| + // The account is the one the console recorded the intent for, not |
| + // the one the subscription's metadata names: an intent id is the |
| + // console's own and unique, and a subscription made by hand that |
| + // names it under another account, or under none, is still a |
| + // subscription naming that intent. Only an intent the console has no |
| + // record of falls back to the account in the metadata, where the |
| + // classification then finds nothing. |
| + accountID, known, err := accountOfIntentTx(tx, intentID) |
| + if err != nil { |
| return err |
| } |
| + if !known { |
| + if err := tx.QueryRow(`SELECT id FROM accounts WHERE sqid=?`, sqid).Scan(&accountID); err != nil { |
| + if errors.Is(err, sql.ErrNoRows) { |
| + return nil |
| + } |
| + return err |
| + } |
| + } |
| var recorded string |
| if err := tx.QueryRow(`SELECT subscription FROM billing_accounts WHERE account_id=?`, accountID).Scan(&recorded); err != nil && !errors.Is(err, sql.ErrNoRows) { |
| return err |
| @@ -729,6 +764,28 @@ func (s *Store) noteSubscriptionSeen(sqid, intentID, subscription string, now ti |
| return tx.Commit() |
| } |
| +// accountOfIntentTx finds the account the console recorded an intent id |
| +// for: the standing intent, else what a purchase consumed, else a |
| +// retirement, else a refusal's resolution. |
| +func accountOfIntentTx(tx *sql.Tx, intentID string) (int64, bool, error) { |
| + for _, q := range []string{ |
| + `SELECT account_id FROM billing_checkout_intents WHERE id=?`, |
| + `SELECT account_id FROM billing_consumed_intents WHERE intent_id=? LIMIT 1`, |
| + `SELECT account_id FROM billing_retired_intents WHERE intent_id=? LIMIT 1`, |
| + `SELECT account_id FROM billing_review_resolutions WHERE subscription=? AND reason='` + billingCheckoutRefusedReason + `' LIMIT 1`, |
| + } { |
| + var accountID int64 |
| + err := tx.QueryRow(q, intentID).Scan(&accountID) |
| + if err == nil { |
| + return accountID, true, nil |
| + } |
| + if !errors.Is(err, sql.ErrNoRows) { |
| + return 0, false, err |
| + } |
| + } |
| + return 0, false, nil |
| +} |
| + |
| // attachSubscriptionTx records a subscription as attached to a refused |
| // intent and reports whether it was not attached before. |
| func attachSubscriptionTx(tx *sql.Tx, accountID int64, intentID, subscription string, now time.Time) (bool, error) { |
| @@ -824,6 +881,8 @@ func (s *Server) readSubscription(ctx context.Context, id string) (*billingstrip |
| return nil, err |
| } |
| if sub.ID == id && sub.LiveMode == s.billing.cfg.LiveMode { |
| + // By the intent it names; the account it names is used only for |
| + // an intent the console has no record of. |
| if err := s.store.noteSubscriptionSeen(sub.Metadata["account_sqid"], sub.Metadata["checkout_intent"], sub.ID, time.Now()); err != nil { |
| return nil, err |
| } |
| @@ -908,8 +967,9 @@ func (s *Server) advanceRefusal(ctx context.Context, intent billingIntent, acct |
| // billingPassRefusals works a bounded number of refused intents after |
| // the pass's other work: of those whose try is due, the ones that have |
| -// been due longest. A refusal that was just tried is due again an hour |
| -// later at the earliest, so one that stays due is reached within a few |
| +// been due longest. A refusal that was just tried is due again a minute |
| +// later at the earliest, and an hour later once it has been tried three |
| +// times (noteExpiryTry), so one that stays due is reached within a few |
| // passes however many others there are; taken in the order they stand, |
| // the same old ones could use the budget for ever. |
| func (s *Server) billingPassRefusals(ctx context.Context, refused []billingIntent, now time.Time) []error { |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index 6d457819b..5247b5e63 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -647,10 +647,12 @@ func (s *Server) startBillingCheckoutQuoted(ctx context.Context, acct *Account, |
| } |
| // The read was recorded (readCheckout). If it contradicted what |
| // is recorded of a purchase that was kept, the intent is disputed |
| - // now: nothing is offered, expired or replaced on it. |
| + // now; and another console may have filed a refusal while the |
| + // read was in flight. Either way nothing is offered, expired or |
| + // replaced on it: the state is read again here, after the read. |
| if st, err := s.store.refusalState(acct.ID, previous.ID); err != nil { |
| return "", err |
| - } else if st.Disputed != 0 { |
| + } else if st.Open || st.Disputed != 0 { |
| return "", errBillingUnderReview |
| } |
| if own, err := s.ownBoundSession(ctx, session, previous, acct, now); err != nil { |
| @@ -712,7 +714,7 @@ func (s *Server) startBillingCheckoutQuoted(ctx context.Context, acct *Account, |
| } |
| if st, err := s.store.refusalState(acct.ID, intent.ID); err != nil { |
| return "", err |
| - } else if st.Disputed != 0 { |
| + } else if st.Open || st.Disputed != 0 { |
| return "", errBillingUnderReview |
| } |
| if own, err := s.ownBoundSession(ctx, session, intent, acct, now); err != nil { |
| @@ -772,13 +774,23 @@ func (s *Server) startBillingCheckoutQuoted(ctx context.Context, acct *Account, |
| if err := s.store.saveBillingCheckout(intent, session.ID, session.URL, session.ExpiresAt); err != nil { |
| return "", err |
| } |
| + // The link is disclosed only for an intent that is not under review |
| + // now: a refusal filed while the session was being made (a |
| + // subscription that names the intent, seen by another console) leaves |
| + // it bound and undisclosed, for the pass to expire. |
| + if st, err := s.store.refusalState(acct.ID, intent.ID); err != nil { |
| + return "", err |
| + } else if st.Open || st.Disputed != 0 { |
| + return "", errBillingUnderReview |
| + } |
| return session.URL, nil |
| } |
| // resolveIntentForDeletion clears a checkout intent that must not block an |
| // account deletion. A stillborn intent (no session, customer not ready) proves |
| -// no session was ever created and is discarded unconditionally — the operator |
| -// is deleting the account, so a never-started attempt must not wedge it. Every |
| +// no session was ever created and is discarded, unless a subscription was |
| +// seen naming it or a refusal review is open on it (discardUnreadyIntent |
| +// refuses both, and the deletion then waits on a person). Every |
| // other resting state is handed to the ordinary pending-intent path: an |
| // expired session is retired, a completed purchase is bound and activated (and |
| // then legitimately blocks the deletion), and a lost response is recovered. |
diff --git a/internal/console/billingservice_test.go b/internal/console/billingservice_test.go| index f0bca5ae7..3b7aef2b5 100644 |
| --- a/internal/console/billingservice_test.go |
| +++ b/internal/console/billingservice_test.go |
| @@ -73,6 +73,9 @@ type fakeBillingStripe struct { |
| // listSecondPageFails makes the session listing answer one page that |
| // says there is more, and fail the next. |
| listSecondPageFails bool |
| + // listRepeatsOnSecondPage makes the listing's second page repeat the |
| + // session of its first. |
| + listRepeatsOnSecondPage bool |
| // otherSubscriptions are further subscription ids the provider knows |
| // (sub_paid always exists), served on the same terms; and |
| // subscriptionSecondItem gives every subscription a second item. |
| @@ -257,6 +260,12 @@ func (f *fakeBillingStripe) serve(w http.ResponseWriter, r *http.Request) { |
| } |
| out = map[string]any{"data": rows, "has_more": false} |
| case r.URL.Path == "/v1/checkout/sessions" && r.Method == http.MethodGet: |
| + if f.listRepeatsOnSecondPage { |
| + // The second page repeats the first page's session, as it |
| + // reads then: broken pagination that still says something. |
| + out = map[string]any{"data": []any{f.session()}, "has_more": r.URL.Query().Get("starting_after") == ""} |
| + break |
| + } |
| if f.listSecondPageFails { |
| if r.URL.Query().Get("starting_after") != "" { |
| w.WriteHeader(http.StatusInternalServerError) |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index 8f4aaf36c..5891150ad 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -954,6 +954,11 @@ func (s *Store) applyBillingStateDated(next billingState, expectedRevision int64 |
| // recorded. |
| func fileBillingReviewTx(tx *sql.Tx, accountID int64, subscription, reason string, observed time.Time) error { |
| at := observed.Unix() |
| + if reason == billingCheckoutRefusedReason || reason == billingFateDisputedReason { |
| + if err := cancelPendingRetirementTx(tx, accountID); err != nil { |
| + return err |
| + } |
| + } |
| _, err := tx.Exec(`INSERT INTO billing_reviews(account_id,subscription,reason,created_at,last_seen_at,occurrences) VALUES(?,?,?,?,?,1) |
| ON CONFLICT(account_id,subscription,reason) DO UPDATE SET resolved_at=0, occurrences=occurrences+1, last_seen_at=MAX(last_seen_at,excluded.last_seen_at)`, |
| accountID, subscription, reason, at, at) |
| @@ -1247,6 +1252,27 @@ func (s *Store) fileCheckoutRefusal(intent billingIntent, sessionID, subscriptio |
| if err := classifySubscriptionTx(tx, intent.AccountID, intent.ID, subscription, observed); err != nil { |
| return err |
| } |
| + // And the refusal itself, when it is of a session and the intent |
| + // went to a purchase's record: this console judged the session |
| + // not the purchase, another recorded it meanwhile, and that |
| + // disagreement is kept against what was consumed. (For an intent |
| + // that was retired or kept, the session is handed in again by the |
| + // caller and classified against that archive: fileRefusal.) |
| + if sessionID != "" { |
| + var usedSession, usedSubscription string |
| + err := tx.QueryRow(`SELECT session_id,subscription FROM billing_consumed_intents WHERE account_id=? AND intent_id=?`, intent.AccountID, intent.ID).Scan(&usedSession, &usedSubscription) |
| + if err != nil && !errors.Is(err, sql.ErrNoRows) { |
| + return err |
| + } |
| + if err == nil { |
| + if usedSession == "" { |
| + usedSession = intent.ID // the key the record and the classification use |
| + } |
| + if err := disputeSessionTx(tx, intent.AccountID, usedSession, intent.ID, "a refusal of session "+sessionID+" was filed after the purchase recorded with "+usedSubscription, observed); err != nil { |
| + return err |
| + } |
| + } |
| + } |
| return tx.Commit() |
| } |
| var open bool |
| @@ -1476,7 +1502,16 @@ func advanceBillingMonthTx(tx *sql.Tx, x billingMonthRecord, state, invoiceID, i |
| return nil |
| } |
| -func checkBillingRetirement(tx *sql.Tx, accountID int64) error { |
| +// An account is not retired while a checkout refusal or a checkout |
| +// dispute is open on it: each is a subscription or a session at the |
| +// provider that a person has not disposed of. That clause is read when the |
| +// retirement is reserved and when it is committed; once committed (the |
| +// point of no return, in this database, before anything outside it is |
| +// written) a review that opens afterwards does not stop the deletion |
| +// finishing, and is listed and resolvable under the deleted account |
| +// instead (the operator doors look accounts up whether or not they are |
| +// deleted). |
| +func checkBillingRetirement(tx *sql.Tx, accountID int64, committed bool) error { |
| if held, err := quantityObligationTx(tx, accountID); err != nil { |
| return err |
| } else if held { |
| @@ -1485,7 +1520,9 @@ func checkBillingRetirement(tx *sql.Tx, accountID int64) error { |
| var blocked bool |
| err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM billing_accounts WHERE account_id=? AND subscription<>'' AND status NOT IN ('canceled','incomplete_expired')) |
| OR EXISTS(SELECT 1 FROM billing_checkout_intents WHERE account_id=?) |
| - OR EXISTS(SELECT 1 FROM billing_months WHERE account_id=? AND state NOT IN ('complete','waived'))`, accountID, accountID, accountID).Scan(&blocked) |
| + OR EXISTS(SELECT 1 FROM billing_months WHERE account_id=? AND state NOT IN ('complete','waived')) |
| + OR (NOT ? AND EXISTS(SELECT 1 FROM billing_reviews WHERE account_id=? AND reason IN (?,?) AND resolved_at=0))`, |
| + accountID, accountID, accountID, committed, accountID, billingCheckoutRefusedReason, billingFateDisputedReason).Scan(&blocked) |
| if err != nil { |
| return err |
| } |
| @@ -1495,6 +1532,55 @@ func checkBillingRetirement(tx *sql.Tx, accountID int64) error { |
| return nil |
| } |
| +// retirementCommitted reports whether the account's retirement has passed |
| +// its point of no return. |
| +func retirementCommitted(tx *sql.Tx, accountID int64) (bool, error) { |
| + var committed bool |
| + err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM billing_retirements WHERE account_id=? AND committed_at<>0)`, accountID).Scan(&committed) |
| + return committed, err |
| +} |
| + |
| +// cancelPendingRetirementTx withdraws a reservation that has not been |
| +// committed: checkout evidence that lands while a deletion is being |
| +// prepared stops it, in the evidence's own transaction. A committed |
| +// retirement is not withdrawn; the evidence stays with the account. |
| +func cancelPendingRetirementTx(tx *sql.Tx, accountID int64) error { |
| + _, err := tx.Exec(`DELETE FROM billing_retirements WHERE account_id=? AND committed_at=0`, accountID) |
| + return err |
| +} |
| + |
| +// commitBillingRetirement is the retirement's point of no return, taken |
| +// after the reservation and before the first write outside this database. |
| +// It re-reads every condition in its own transaction, so a checkout review |
| +// that opened since the reservation (and withdrew it) refuses it here, and |
| +// nothing outside has been touched. A retirement already committed (a |
| +// retry after an ambiguous failure outside) passes whatever has opened |
| +// since. |
| +func (s *Store) commitBillingRetirement(accountID int64, now time.Time) error { |
| + tx, err := s.db.Begin() |
| + if err != nil { |
| + return err |
| + } |
| + defer tx.Rollback() |
| + var reserved, committed bool |
| + if err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM billing_retirements WHERE account_id=?), EXISTS(SELECT 1 FROM billing_retirements WHERE account_id=? AND committed_at<>0)`, accountID, accountID).Scan(&reserved, &committed); err != nil { |
| + return err |
| + } |
| + if committed { |
| + return nil |
| + } |
| + if !reserved { |
| + return errBillingRetirement |
| + } |
| + if err := checkBillingRetirement(tx, accountID, false); err != nil { |
| + return err |
| + } |
| + if _, err := tx.Exec(`UPDATE billing_retirements SET committed_at=? WHERE account_id=?`, max(now.Unix(), 1), accountID); err != nil { |
| + return err |
| + } |
| + return tx.Commit() |
| +} |
| + |
| // Reserve before the bucket tombstone. Keep the reservation on an ambiguous |
| // write failure: a retry may finish retirement, but cannot open a new checkout. |
| func (s *Store) reserveBillingRetirement(accountID int64, now time.Time) error { |
| @@ -1503,7 +1589,11 @@ func (s *Store) reserveBillingRetirement(accountID int64, now time.Time) error { |
| return err |
| } |
| defer tx.Rollback() |
| - if err := checkBillingRetirement(tx, accountID); err != nil { |
| + committed, err := retirementCommitted(tx, accountID) |
| + if err != nil { |
| + return err |
| + } |
| + if err := checkBillingRetirement(tx, accountID, committed); err != nil { |
| return err |
| } |
| if _, err := tx.Exec(`INSERT OR IGNORE INTO billing_retirements(account_id,started_at) VALUES(?,?)`, accountID, now.Unix()); err != nil { |
| @@ -1518,6 +1608,35 @@ func (s *Store) reserveBillingRetirement(accountID int64, now time.Time) error { |
| // touching the bucket). An ambiguous failure AFTER a write may have begun |
| // must keep the reservation, so that path never calls this: a retry can then |
| // finish retirement, but no second checkout can open in the meantime. |
| +// accountEvenIfDeleted and accountBySqidEvenIfDeleted are the operator |
| +// doors' lookups: a billing review of a deleted account is still a person's |
| +// to see and resolve. |
| +func (s *Store) accountEvenIfDeleted(id int64) (*Account, bool, error) { |
| + var a Account |
| + var created, deleted int64 |
| + err := s.db.QueryRow(`SELECT id, sqid, name, created_at, deleted_at FROM accounts WHERE id = ?`, id).Scan(&a.ID, &a.Sqid, &a.Name, &created, &deleted) |
| + if errors.Is(err, sql.ErrNoRows) { |
| + return nil, false, nil |
| + } |
| + if err != nil { |
| + return nil, false, err |
| + } |
| + a.CreatedAt = time.Unix(created, 0).UTC() |
| + return &a, deleted != 0, nil |
| +} |
| + |
| +func (s *Store) accountBySqidEvenIfDeleted(sqid string) (*Account, bool, error) { |
| + var id int64 |
| + err := s.db.QueryRow(`SELECT id FROM accounts WHERE sqid = ?`, sqid).Scan(&id) |
| + if errors.Is(err, sql.ErrNoRows) { |
| + return nil, false, nil |
| + } |
| + if err != nil { |
| + return nil, false, err |
| + } |
| + return s.accountEvenIfDeleted(id) |
| +} |
| + |
| func (s *Store) releaseBillingRetirement(accountID int64) error { |
| _, err := s.db.Exec(`DELETE FROM billing_retirements WHERE account_id=?`, accountID) |
| return err |
diff --git a/internal/console/ownership.go b/internal/console/ownership.go| index 599ce6a6f..7fb896ac1 100644 |
| --- a/internal/console/ownership.go |
| +++ b/internal/console/ownership.go |
| @@ -154,6 +154,18 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { |
| http.Redirect(w, r, "/a/"+acct.Sqid+"/settings", http.StatusSeeOther) |
| return |
| } |
| + // The point of no return, in the database, before anything outside it: |
| + // a checkout review that opened since the reservation (and withdrew |
| + // it) refuses here, and the reservation it withdrew is not re-made. |
| + if err := s.store.commitBillingRetirement(acct.ID, time.Now()); err != nil { |
| + log.Printf("console: account retirement: %s", billingDiagnostic(err)) |
| + if rerr := s.store.releaseBillingRetirement(acct.ID); rerr != nil { |
| + log.Printf("console: release retirement reservation %s: %v", acct.Sqid, rerr) |
| + } |
| + s.setFlash(w, "err", deletionAdvice(err)) |
| + http.Redirect(w, r, "/a/"+acct.Sqid+"/settings", http.StatusSeeOther) |
| + return |
| + } |
| // The bucket first, and the deletion is abandoned if it will not take. |
| // |
| // This console's rows are not where account identity lives — every console |
| @@ -728,7 +740,11 @@ func (s *Store) DeleteAccount(accountID int64, now time.Time) error { |
| return err |
| } |
| defer tx.Rollback() |
| - if err := checkBillingRetirement(tx, accountID); err != nil { |
| + committed, err := retirementCommitted(tx, accountID) |
| + if err != nil { |
| + return err |
| + } |
| + if err := checkBillingRetirement(tx, accountID, committed); err != nil { |
| return err |
| } |
| var apps int |
diff --git a/internal/console/store.go b/internal/console/store.go| index 3e66345b0..288927f2b 100644 |
| --- a/internal/console/store.go |
| +++ b/internal/console/store.go |
| @@ -1363,7 +1363,11 @@ func (s *Store) ReapAccount(accountID int64, now time.Time) error { |
| return err |
| } |
| defer tx.Rollback() |
| - if err := checkBillingRetirement(tx, accountID); err != nil { |
| + // The reap follows a retirement the deployment's manifest already |
| + // records (made here after its commit, or on another console): an open |
| + // checkout review does not stop it, and stays listed and resolvable |
| + // under the deleted account. |
| + if err := checkBillingRetirement(tx, accountID, true); err != nil { |
| return err |
| } |
| for _, stmt := range []string{ |