Review twenty-two answered: the rule's edges (account retirement, a refusal filed late, a click racing a filing) and what it costs an honest buyer stated as a gate
Round twenty-two: Opus accept with fixes (the named-subscription rule holds; review eighteen's P2 closed for deletion and consumption; one P2 on what the rule costs an honest buyer; four P3s), Astra reject (three P2s at the rule's edges, one P3). - An account is not retired while a checkout refusal or a checkout dispute is open on it (Astra 1). - 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 (Astra 2). - A click reads the intent's state again after each provider read and before it discloses a new link; an open review stops it (Astra 3). - The listing client returns everything a broken page decoded with its error, and the wrapper records it all (Astra 4). - A subscription is recorded under the account the console recorded its intent for, by the intent id, whatever its metadata names (Opus P3-2); the pass does not fail on a hold it caused (P3-3); one dispute key for an intent consumed with no session (P3-4); the second exception to the rule is stated (P3-1). - Opus P2-1 is not code: an honest buyer whose provider left a dead subscription naming the intent is held with no stage-one disposition. Whether the provider ever makes a subscription before its session completes is the first test-mode check and a gate on enabling; whether incomplete_expired is nothing owed is a decision. The dispositions, header and design say so. Nine mutants in a tree copy; one needed a sharper test. Review eighteen's P2 stays recorded as open until a round accepts this.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
12 files changed,
+896
−21
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/DISPOSITIONS-multi-currency.md+23 −2docs/superpowers/specs/OPUS-REVIEW-multi-currency-review22.md+183 −0internal/billingstripe/catalog.go+14 −2internal/billingstripe/catalog_test.go+29 −0internal/console/billingnamed_test.go+280 −0internal/console/billingpass.go+6 −1internal/console/billingretirement.go+65 −11internal/console/billingservice.go+14 −3internal/console/billingservice_test.go+9 −0internal/console/billingstore.go+28 −1
CI failed — 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..e0ac7bd07 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. **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/DISPOSITIONS-multi-currency.md b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md| index 0584fcc8b..fa6ff3b7f 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,30 @@ 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` | | |
| +| 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. |
| + |
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/internal/billingstripe/catalog.go b/internal/billingstripe/catalog.go| index 6978681d8..e28742630 100644 |
| --- a/internal/billingstripe/catalog.go |
| +++ b/internal/billingstripe/catalog.go |
| @@ -161,13 +161,25 @@ 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 |
| } |
| + // A page that repeats a session, or carries one with no usable |
| + // id, is broken pagination and the listing is refused. What the |
| + // page did say is still returned with that error: a session seen |
| + // twice is two answers about it, and the second may differ. |
| + broken := false |
| for _, session := range page.Data { |
| - if !validID(session.ID) || seen[session.ID] { |
| - return out, &Error{Code: CodeBrokenPagination} |
| + if !validID(session.ID) { |
| + broken = true |
| + continue |
| + } |
| + if seen[session.ID] { |
| + broken = true |
| } |
| seen[session.ID] = true |
| out = append(out, session) |
| } |
| + if broken { |
| + return out, &Error{Code: CodeBrokenPagination} |
| + } |
| if len(out) > 10000 { |
| return out, &Error{Code: CodeTooManyRecords} |
| } |
diff --git a/internal/billingstripe/catalog_test.go b/internal/billingstripe/catalog_test.go| index 7c041ab07..443faddc6 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,34 @@ func TestListCheckouts_ALaterPagesFailureKeepsTheEarlierPages(t *testing.T) { |
| } |
| } |
| +// 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/billingnamed_test.go b/internal/console/billingnamed_test.go| index 3e21e25a0..2aefc3a13 100644 |
| --- a/internal/console/billingnamed_test.go |
| +++ b/internal/console/billingnamed_test.go |
| @@ -673,3 +673,283 @@ 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) |
| + } |
| +} |
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..edfff3821 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" |
| @@ -638,12 +651,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 +719,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 +727,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 +759,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 +876,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 |
| } |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index 6d457819b..670103862 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,6 +774,15 @@ 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 |
| } |
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..09e703d57 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -1247,6 +1247,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 = sessionID |
| + } |
| + 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,6 +1497,10 @@ func advanceBillingMonthTx(tx *sql.Tx, x billingMonthRecord, state, invoiceID, i |
| return nil |
| } |
| +// 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, and once the account is gone |
| +// its reviews are listed under no account and resolved through no door. |
| func checkBillingRetirement(tx *sql.Tx, accountID int64) error { |
| if held, err := quantityObligationTx(tx, accountID); err != nil { |
| return err |
| @@ -1485,7 +1510,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 EXISTS(SELECT 1 FROM billing_reviews WHERE account_id=? AND reason IN (?,?) AND resolved_at=0)`, |
| + accountID, accountID, accountID, accountID, billingCheckoutRefusedReason, billingFateDisputedReason).Scan(&blocked) |
| if err != nil { |
| return err |
| } |