amadan
Review twenty-four's two reports in the tree
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 files changed,
+293
−0
docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review24.md+183 −0docs/superpowers/specs/OPUS-REVIEW-multi-currency-review24.md+110 −0
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review24.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review24.md| new file mode 100644 |
| index 000000000..e3675f2f5 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review24.md |
| @@ -0,0 +1,183 @@ |
| +All platform references below are at **`e152df30`**; pricing-site references are at **`01b62f7`**. This was source-only review. Nothing was modified or tested, and no network commands were run. |
| + |
| +**Twenty-third review, finding by finding** |
| + |
| +| Item | Closed? | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **Opus P3-1:** late refusal uses another key when the consumed intent has no session | **Code fixed** at `internal/console/billingstore.go:1269`: the fallback is now the intent ID. | **Yes, the existing tests would still pass if this particular line were reverted.** `billingnamed_test.go:938` tests subscription classification, while `:758` tests late refusal with a nonempty archived session. Neither exercises this fallback. | |
| +| **Opus P3-2:** deleted accounts’ reviews lose their account and resolution door | **Closed for the review listing and door.** `billingoperator.go:206` retains the account and marks `account_deleted`; `:343` finds deleted accounts. | **No.** `billingnamed_test.go:1066` requires deletion, the attributed listing row, and a held **409 rather than 404**. It does not prove that a dispute can be resolved. | |
| +| **Opus P3-3:** recovery’s hold filter has no test | **Closed.** `billingnamed_test.go:1144` exercises lost-create recovery, a wrong-currency session, expiry, and the resulting named-subscription hold. | **No.** Removing the recovery filter at `billingpass.go:297` makes the test’s nil-error assertion fail. | |
| +| **Opus claim 1:** unready cleanup is unconditional | **Corrected**, `billingservice.go:789`. Both the open-review and named-subscription exceptions are stated. | Not applicable: comment correction. | |
| +| **Opus claim 2:** the next retry is at least an hour away | **Mostly corrected**, but the replacement has an off-by-one description; see contradicted claims below. | No behavioral change was made. | |
| +| **Opus claim 3:** the dispositions cite a decision file as though it exists here | **Corrected:** the citation now identifies the launch branch, `DISPOSITIONS-multi-currency.md:500`. | Not applicable. | |
| +| **Opus claim 4:** no subscription before completion means no cost to an honest buyer | **Reported corrected elsewhere, not verifiable at this pinned commit.** `DISPOSITIONS-multi-currency.md:525` describes the launch-branch edit. The second-subscription limitation is stated in this branch’s retirement design. | Not applicable; the external edit cannot be certified from this tree. | |
| +| **Astra 1, P2:** evidence after reservation can leave the account externally retired and locally blocked | **The reported normal-path ordering is fixed, but the retirement boundary remains incomplete.** Failure cleanup can erase another console’s committed retirement: Finding 1. | **No** for removal of the new reservation, commit, or post-commit checks exercised at `billingnamed_test.go:965`. **Those tests still pass with Finding 1.** | |
| +| **Astra 2, P3:** one malformed session drops its otherwise decodable page | **Closed**, `internal/billingstripe/catalog.go:153`. Sessions are decoded separately and successful decodes accompany the error. | **No.** `catalog_test.go:333` requires both the preceding and following valid session alongside the decode error. | |
| + |
| +The gate failure was an obsolete assertion, not evidence that the new attribution rule was wrong. Before round twenty-two’s change, `TestABoundSessionReadBackAsNobodysIsReviewed` reached the unproven-expiry hold because the incorrectly attributed subscription had not been attached. That expectation matched the previous implementation, including its attribution defect. At `2618403f`, the subscription correctly attached to the account recorded for the intent, and `held()` checks that obligation first. |
| + |
| +The updated test at `billingcurrency_test.go:3512` correctly requires the `sub_paid` hold, then deliberately removes the attachment to isolate the unproven-expiry hold at `:3516`. Reverting the attribution fix would fail its first assertion. Keeping the old expectation would fail against the corrected implementation. **The test correction is closed; I did not run the current gate.** |
| + |
| +On the previous severity disagreement, **Astra’s P2 reading was right**. The old reservation check did not govern publication of the authoritative tombstone. Opus’s conclusion that the complete lifecycle boundary was closed at `2618403f` was too broad. The recovery diagnostic and defensive empty-session key remain appropriately P3. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P2 — A failed deletion request can remove another console’s committed retirement and reopen checkout during tombstone publication.** |
| + |
| + **Where:** `internal/console/ownership.go:160`, `:162`, `:190`; `internal/console/billingstore.go:1559`, `:1640`. Checkout’s reservation check is at `billingstore.go:363`. |
| + |
| + **Concrete scenario:** |
| + |
| + 1. Two consoles share the database and both reserve deletion of an eligible account. |
| + 2. Both enter `commitBillingRetirement` and read the reservation before either commits. |
| + 3. Console A commits `committed_at` and starts the external tombstone write. |
| + 4. Console B’s deferred read transaction cannot upgrade its stale WAL snapshot and returns a busy/snapshot error. Its transaction rolls back. |
| + 5. B’s handler calls `releaseBillingRetirement`. Its unconditional `DELETE` removes **A’s committed reservation**. |
| + 6. Before A finishes its external write, a checkout click creates a new intent and payable session: `beginBillingCheckout` now sees an undeleted account with no retirement reservation. |
| + 7. A publishes the tombstone. Both `DeleteAccount` and `ReapAccount` refuse the new standing intent. |
| + |
| + This requires no unusual Stripe behavior. One connection per `Store` at `store.go:65` does not serialize the read snapshots of two separate consoles. |
| + |
| + A narrower variant needs only late dispute evidence after step 5: A publishes the tombstone, then local deletion refuses because the commit marker has disappeared. The new reap can eventually finish that dispute-only variant; it **cannot** finish the new-standing-intent variant. |
| + |
| + The `errNoReleaseStore` cleanup at `ownership.go:190` has the same authority problem: one console’s knowledge that *it* made no external write does not establish that another console sharing the reservation made none. |
| + |
| + **Smallest acceptable fix:** release only pending rows, with `committed_at=0`. Check for missing release storage before committing retirement so that this preflight failure never requires removing a committed row. Add a regression covering a losing request’s cleanup after another request commits, and require both the marker to survive and new checkout to remain blocked. |
| + |
| + The new sequential test calls neither failing handler cleanup nor competing commits. Its passing assertions do not cover this ordering. |
| + |
| +2. **P3 — Rejected-event listings still omit a deleted account’s recorded identity.** |
| + |
| + **Where:** `internal/console/billingoperator.go:312`, particularly `:314`. |
| + |
| + **Concrete scenario:** a rejected event is recorded with an account ID. The account is subsequently deleted. `GET /api/billing/rejected` still returns the event, but `AccountByID` filters out the deleted account, so its `account` field disappears. |
| + |
| + The separate review listing now handles this correctly. This is a smaller attribution gap, not a recurrence of the invisible-review P2. |
| + |
| + **Smallest acceptable fix:** use the deleted-account-aware lookup for rejected rows too, preserving the recorded account sqid. Extend the listing regression with a soft-deleted account. |
| + |
| +**The retirement boundary and remaining lifecycle checks** |
| + |
| +The intended point of no return is sound **while its committed marker remains durable**: |
| + |
| +- Evidence first: `fileBillingReviewTx` withdraws the pending reservation in the evidence transaction (`billingstore.go:957`); commitment then refuses. |
| +- Commitment first: later evidence stays recorded; the committed retirement permits local deletion despite the newly opened checkout review. |
| +- Tombstone first, local process dies: `ReapAccount` accepts that the manifest already retired the account and preserves its billing records and reviews (`store.go:1366`). |
| +- Deleted account: the review retains its account and reaches the operator door. |
| + |
| +Finding 1 breaks the premise that the committed marker remains durable. Consequently, **I would not yet close review eighteen’s P2 as a whole**. |
| + |
| +“Resolvable” also needs precision. The deleted account’s review is now **listed and reachable**. A checkout dispute still has no stage-one disposition: `resolveBillingReview` returns a held conflict at `billingstore.go:1133`. The new test explicitly expects that result. The visibility fix does not implement settlement, and should not claim to. |
| + |
| +An older binary has no commitment step and can still publish a tombstone without following this protocol. The migration’s default `committed_at=0` does not teach it otherwise. The documented rollout rule—stop older consoles before starting this build and do not roll them back—remains necessary. |
| + |
| +The intent-level invariant supported by the inspected code is: |
| + |
| +> A recorded naming subscription survives consumption as accepted purchase identity or a dispute; expiry and cleanup preserve intents with recorded obligations or open refusals; operator retirement requires the recorded proof and absence of obligations in its own transaction. |
| + |
| +The complete mutation inventory is: |
| + |
| +| Door | Recorded evidence and subsequent observations | |
| +|---|---| |
| +| **Ordinary expiry — `billingstore.go:553`** | Pass, click and refusal handling require identity/proof and the exact intent. An observed naming subscription causes a refusal instead of deletion. Open review or dispute prevents deletion. | |
| +| **Unready cleanup — `billingstore.go:604`** | Pass, replacement checkout and deletion preparation require never-created state. A named subscription or open refusal prevents cleanup. | |
| +| **First-purchase consumption — `billingstore.go:890`** | Exact standing-intent consumption, billing state, assignments, event, entitlement work, consumption archive and disputes about other subscriptions commit together. Refusal rolls everything back. Renewals consume no intent. | |
| +| **Operator retirement — `billingstore.go:1208`** | Deletes exactly the reviewed intent, with its retirement archive, resolution evidence and quantity-event outcome in the same transaction. | |
| +| **Session binding — `billingstore.go:458`, `:461`** | Both writes are inside `bindIntentSession`; another purchase’s session or one conflicting with the review reference is refused. Rebinding the same valid session remains possible. | |
| +| **Account deletion/reap** | Soft deletion; no checkout-intent cascade. Billing records survive. Finding 1 remains at the external retirement boundary. | |
| + |
| +The two documented no-archive exceptions remain: ordinary expiry and never-created cleanup, each with **nothing previously observed naming the intent**. A subscription first observed after either can become only a rejected event. Consumption and protocol retirement retain archives and do not share that exception. |
| + |
| +**Other requested checks** |
| + |
| +| Scope | Source conclusion | |
| +|---|---| |
| +| **A: one GBP purchase** | With `?country=IN¤cy=gbp`, the page selects India’s band and GBP (`billingpage.go:291`). A valid posted form, including the required postal information, passes effective-quote comparison (`billinghandlers.go:81`); checkout derives quantity and `PriceTerms` (`billingservice.go:494`). `CreateCheckout` sends GBP with Adaptive Pricing disabled (`billingstripe/client.go:589`). Binding and reconciliation verify ownership and purchase identity; state and assignments retain the book/currency (`billingstore.go:805`). History (`billinginvoicestore.go:524`), allowance (`plans/month.go:100`), calculation (`billingcalculation.go:119`), invoice requests/recovery/verification (`billinginvoice.go:856`, `:958`) and quantity updates (`billingquantity.go:1008`, `:1099`) preserve GBP. I found no conversion or silent EUR fallback in this trace. | |
| +| **B, M: one form** | Present currency must be exactly one supported value; empty, repeated, uppercase, padded or unsupported values return 400 on both doors (`billinghandlers.go:176`). An absent control supplies no explicit choice. The effective country-default currency must still match the signed quote. Changed inputs, erased choice flags and fabricated quote fields cannot authorize an unseen quote. A stale tab may buy its own still-valid quote; another tab’s cookies do not alter its posted purchase. Legacy no-book behavior remains EUR. | |
| +| **Quote redirect** | The quote door checks session, form token and membership, then constructs the account’s fixed billing path with encoded parameters (`billinghandlers.go:165`, `:197`). Posted values cannot choose another destination or initiate checkout. | |
| +| **Q, U, Z: signature** | The HMAC key derives from the server billing secret, not the disclosed CSRF value (`billingpage.go:446`). It covers session token, account, book, country, band, currency, choice flag and member count. Cross-session/account/book/token reuse fails. Plan is not signed because the page quotes the offered plans; quantity derives from the signed membership count and plan minimum. The count is compared again and validated under the membership write lock. Postal information affects tax/location, not the displayed pre-tax book amount. | |
| +| **C, O: cookies and held currency** | Present-empty clears; absent falls back; invalid changes no cookie (`billingcurrency.go:43`). Thus an invalid link can leave an older cookie available on a later parameter-absent visit; it does not silently authorize checkout. Held currency overrides URL/cookie suggestions and is offered alone (`billingpage.go:298`), including legacy EUR. Service and store enforce the hold after cancellation too. | |
| +| **E, N: allowance barrier** | The trigger at `billingstore.go:208` uses positive paid overlap ending at the next assignment, paid-through instant or month end, matching `AllowanceForMonth`. It orders both assignment tables by effective time and revision. Same-second supersession contributes no span; free/inactive rows do not count. Renewal and quantity writers preserve provider `period_end` as paid-through, distinct from effective time (`billingstore.go:842`). I found neither requested valid-history counterexample. | |
| +| **H, T: migration** | The refined trigger is present before `retireEarlierBarrierTrigger` verifies it and transactionally removes the old name (`billingstore.go:1001`). Missing new trigger leaves the old one; another current startup is idempotent. A prohibited earlier feature binary can recreate the wider trigger. Barriers prevent particular old writes; they do not make rollback generally supported. | |
| +| **F: inclusions** | The page divides `PriceFor`’s pooled allowance by charged quantity (`billingpage.go:395`). Disabled Personal uses its valid one-member quantity; teams above minimum use their charged quantity. Current factors give exact displayed shares at the tested precision. `billingcurrency_test.go:2554` checks larger quantities and pins band-A sentences. | |
| +| **G, J: book and export** | Validation covers dimensions, duplicates, rounding, overflow and legacy-EUR agreement (`pricebook.go:140`, `:231`). Catalog configuration requires complete sets and unique provider IDs (`cmd/console/billing.go:99`). Provisioning and console checks agree on own-only expanded `currency_options`. The export contains public prices, resources and country mappings, without credentials, customer data or Stripe catalog identities (`pricebook.go:714`). | |
| +| **K: pricing site** | The pinned export matches the platform export. Rates retain micro-unit precision; inclusion scaling and exactness checks derive from exported values (`copy-review/pricing/build.mjs:91`). Country options/defaults come from the book. Links always contain both parameters and carry currency only when chosen (`src/site.js:27`). Locale-only display suggestions intentionally are not transmitted as choices. The first-line draft marker and exact `PRICEBOOK_ALLOW_DRAFT=1` check agree with the checker. Approved numeric tokens are checked against the export. | |
| +| **MM, QQ: site tests** | Whole-token output checks cover the rendered slots across currencies (`test/pricing-pricebook.test.mjs:202`), and `hack/money.mjs` is the exercised formatter. A wrong literal fails; a literal exactly equal to the expected current value passes. An output-equality test cannot establish that an equal literal was not substituted, and the source acknowledges that limit. | |
| +| **X, AA, CC: quantity history** | `quantityOutcomesSQL` supplies the guard, startup detection and obligation checks (`billingquantityaudit.go:20`, `:42`, `:61`). The healing sweep is gone. Startup does not clear pre-history or clock-rollback gaps. | |
| +| **YY, ZZ, BBB, CCC: operator records** | Old omitted outcomes remain `retire`; exact replay compares actor, note and outcome. References and resolution evidence survive. Rejected events are operator-only, newest first, bounded to 200, and rows are drained before account lookups (`billingoperator.go:284`). Finding 2 remains for deleted-account attribution. The list is not a ledger of every interactive error or retry. | |
| + |
| +For **ownership and refusal paths**, `ownSession` checks mode, intent, both account identifiers, known customer and bound ID (`billingservice.go:273`). `intentSession` then checks purchase fields. Both create sites and listing recovery refuse foreign unbound sessions without expiring them. Expected-intent binding checks the expected account before deriving an account from provider metadata. |
| + |
| +There is an intentional exception for a **previously bound session** with contradictory metadata: the console files the review, then may expire its **own recorded bound ID**. This neither authorizes expiry of an ID supplied by the contradictory response nor accepts that response as retirement proof. Given a previously disclosed payable link, expiring the pinned ID is the justified reading of the earlier disagreement. An unowned create/listing result remains open at the provider because the console has no equivalent binding authority. |
| + |
| +Binder and reconciliation compare the first purchase consistently. Owned completed mismatches file the same intent-keyed review; repeated reads do not create repeated open occurrences. Reconcile attaches an offending subscription with the refusal before advancing expiry. Failure or process death leaves the review standing; a completion winning the race is classified from the subsequent read. |
| + |
| +A session that has not completed, or has completed without naming its subscription yet, causes retry rather than premature refusal. The subscription read has already been recorded as naming the intent. Therefore a hand-made subscription is not forgotten while that association remains unknown. Once the session names its own subscription, a different one is refused. Renewals and late events of the recorded subscription do not consume a newer intent. |
| + |
| +For **DDD–JJJJ**, the remaining stage-one behavior is conservative and explicit: |
| + |
| +- Fate is written once; contradictory identity or terminal evidence creates a durable dispute. |
| +- Resolution reads fate, attachments, protocol and dispute state transactionally. A stale caller cannot release newly recorded obligations. |
| +- `keep` requires the owned, matching, completed and acceptably paid bound session. It does **not** validate all subscription terms itself; reconciliation can reopen the refusal. That limitation is accurately stated in “Stage one as built.” |
| +- Other subscriptions seen after `keep` reopen the standing refusal or become archived-purchase disputes after consumption. |
| +- Protocol-zero reviews, disputes, and attached obligations remain held. A status such as `incomplete_expired` alone does not settle them. |
| +- Read, storage or expiry failures remain retryable; settled identity rejections are acknowledged only after required classification. |
| +- The delete barrier is recreated transactionally. It does not supply all protocol protections to an older binary. |
| +- The refusal turn has its own deadline, inherits caller cancellation, has a three-attempt budget and selects longest-due first. This avoids the reported fixed-order starvation, not arbitrary-workload completion guarantees. |
| + |
| +All inspected session/subscription reads—checkout/resume, recovery, binder, reconciliation, pass, operator keep/advance, quantity worker and invoice checks—use `readCheckout`, `listCheckouts` or `readSubscription` (`billingretirement.go:841`). Store failures return no usable object. Successfully decoded listing entries are recorded before the listing error reaches its caller. |
| + |
| +The exclusions remain meaningful: create/expiry responses do not establish fresh fate; pre-refusal session reads do not create a refused-purchase record; identity-less/open reads before any fate record no contradiction. Those exclusions keep a case held rather than authorize retirement. Wrong-request-ID answers record nothing and are refused by callers. |
| + |
| +The structural guard catches selector uses through receiver aliases, method values, method expressions and other receiver types with the classified method names. It also inventories exported client methods. Its wrapper-order check is **lexical**, not control-flow proof; a dead or conditional recording call could satisfy it. No such production bypass was found. |
| + |
| +**Test and mutation conclusions** |
| + |
| +The targeted source assertions support the reported fixes, with limits that matter here: |
| + |
| +- The four formerly surviving round-twenty mutations now have distinguishing cases: identity clauses changed independently; an incomplete additional subscription reopening after `keep`; the binder’s sole read recording the refusal’s fate; and a contradiction appearing only on the second resume read. |
| +- The attachment-failure regression now fails inside `fileCheckoutRefusal`, so splitting its review and attachment transactions would fail the test. |
| +- The fake’s separate second session and late retired-completion assertions distinguish incorrectly recording the retired purchase. |
| +- The historical-retirement helper represents pre-protocol state; it does not establish that current code can legitimately produce that state. |
| +- Review-three’s default-as-explicit-choice mutant remains behaviorally equivalent under the matching quote. Review-four’s provisioning tests inspect script text; they do not execute its jq or provider calls. Successful migration does not simulate every crash ordering. Removing an exactness assertion over already-exact data need not fail a test. |
| +- The eleven round-twenty-three mutation descriptions have corresponding distinguishing assertions for the changes they describe. “Late refusal not kept” is a broader mutation than reverting the empty-session fallback, which remains untested. |
| +- **Sequential store tests do not cover the losing-transaction cleanup in Finding 1.** SQLite’s single writer does not make separately read snapshots and subsequent handler cleanup equivalent to the tested sequence. |
| +- I found no additional shared-cookie-jar, temporary-root or calendar-dependent assertion defect. I cannot certify executed mutant counts or a green gate from source. |
| + |
| +For honest buyers, I found no additional provider-established regression relative to `b260ac8e`. Duplicate own events remain idempotent; ordinary renewals and cancellation do not become disputes. A subscription observed before completion is retained and retried. If abandonment leaves that subscription behind, or the provider replaces it with another ID, the conservative hold can become unresolvable in stage one. Those provider behaviors remain rehearsal gates, not facts established by the fake. |
| + |
| +**Claims the code contradicts** |
| + |
| +1. **A committed retirement is a point of no return.** `billingstore.go:1552` and the retirement design at `:128` overstate this while `releaseBillingRetirement` can delete a committed row. Finding 1 is the concrete counterexample. |
| + |
| +2. **Deleted-account reviews are “resolvable.”** They are attributable and reachable, but a dispute still returns 409 at `billingstore.go:1133`. The test at `billingnamed_test.go:1119` proves that refusal. Say “listed and reachable through the resolution door; existing stage-one holds still apply.” |
| + |
| +3. **The retry becomes hourly after three attempts.** `billingretirement.go:971` is off by one. The SQL at `:803` schedules 60, 300 and 900 seconds after the first three attempts; hourly scheduling starts after the fourth. “Within a few passes however many others” also promises more than a budget of three establishes. |
| + |
| +4. **A late refusal “files nothing.”** The comment at `billingstore.go:1216` contradicts the archive classification and consumed-purchase dispute writes at `:1244`. |
| + |
| +5. **Only `keep` reaches the provider.** `billingoperator.go:21` is stale: `retire` also calls `advanceRefusal` at `:380`. |
| + |
| +The claimed launch-branch enablement wording correction is not independently verifiable from the requested commit. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +1. **Catalog identity:** exercise all 36 prices, exact amounts/currencies, tax behavior, metadata and expanded own-only `currency_options`. Confirm provisioning and console accept the same valid shapes and reject mismatches. |
| + |
| +2. **Checkout choices:** buy India plus explicit GBP, country defaults, cleared suggestions and held legacy EUR. Confirm the shown base amount, disabled Adaptive Pricing, session/subscription currency and stored purchase identity agree. |
| + |
| +3. **Event and payment ordering:** run declined-and-retried cards, abandoned 3-D Secure and delayed methods. Observe when subscriptions exist, when sessions name them, and whether one intent can receive multiple subscription IDs. This decides whether the conservative hold affects honest buyers. |
| + |
| +4. **Outstanding obligations:** inspect payments, pending collection, invoices and refunds for `incomplete_expired` and canceled subscriptions. Establish the evidence needed for the owner’s settlement decision; subscription status alone is insufficient. |
| + |
| +5. **Expiry versus completion:** race retained payment links against expiry, including timeouts and repeated idempotency keys. Verify a fresh GET distinguishes proven expiry from a winning completion and that an expired session cannot subsequently collect. |
| + |
| +6. **Identity and recovery:** verify create/GET/list/expiry identity fields, metadata edits, lost responses, original-key replay, pagination and key-retention expiry. Confirm session IDs are not reused. |
| + |
| +7. **GBP overages and quantities:** exercise invoice creation, lost-response recovery, item/final-line verification, payment and quantity/proration changes. Every amount and recovered request must remain GBP, with correctly dated allowances. |
| + |
| +8. **Legacy and ended purchases:** verify unchanged legacy EUR requests/hashes, renewal/cancellation, customer reuse in the held currency and historical invoice preservation. Establish which canceled-subscription fields can actually change. |
| + |
| +**OOOO:** acceptance still requires the retirement-release fix and its distinguishing regression. What remains is therefore not only rehearsal, decisions and owner approvals. |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review24.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review24.md| new file mode 100644 |
| index 000000000..045e60b34 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review24.md |
| @@ -0,0 +1,110 @@ |
| +**Verdict: accept with fixes.** The point of no return answers Astra's P2 for this binary. I found no P1 or P2. Four P3s and two claims remain, and none of them gates the round. On that basis I would count review eighteen's P2 as closed at `e152df30`. What remains is decision 15, the test-mode rehearsal, the roll rule and the owner's approvals. |
| + |
| +This was a source-only review with `git show e152df30:`. I modified nothing and ran nothing. The delta `2618403f..e152df30` touches no pricing, form, quote, cookie, trigger, export or site code, so the conclusions on A–K, L–ZZZ2 and the site at `01b62f7` stand as last reported. |
| + |
| +## Round twenty-three, finding by finding |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Astra 1 (P2):** a dispute committed between the reservation and the tombstone let the tombstone publish | **Closed for this binary.** `commitBillingRetirement` (`billingstore.go:1559-1583`) re-reads every condition and marks the row committed. The handler calls it before `tombstoneAccount` (`ownership.go:160`). `fileBillingReviewTx` (`billingstore.go:957-961`) withdraws an uncommitted reservation in the evidence's own transaction. That function is the only writer of `billing_reviews` (one `INSERT … ON CONFLICT`), so no refusal or dispute can bypass it. | **No** for the store. Case 1 fails without the withdrawal, and case 3 fails without the `committed` bypass in `DeleteAccount`. **But the handler's own call at `ownership.go:160` is untested** (P3-1 below). | |
| +| **Astra 2 (P3):** one malformed session discarded its page | **Closed.** `catalog.go:153-193` decodes each element on its own and returns the ones that decoded with `CodeDecodeError`. | **No.** The old code failed inside `c.do` and returned an empty `out`. | |
| +| **Opus P3-1:** the third key | **Closed** (`billingstore.go:1269`: `intent.ID`). | No test. The state is unreachable, because the record needs a bound session. This is acceptable as a defensive fix. | |
| +| **Opus P3-2:** a deleted account's review was invisible | **Closed for visibility.** The listing (`billingoperator.go:205-209`) and the door (`:343`) use `accountEvenIfDeleted`. `ReapAccount` and `DeleteAccount` only mark `deleted_at` and never remove the row, so the lookup always finds it. | **No.** Case 3 asserts `account_deleted` and a 409 rather than a 404. The "resolvable" wording is a claim the code contradicts (claim 1). | |
| +| **Opus P3-3:** the recovery site's filter was untested | **Closed.** `TestThePassDoesNotFailOnAHoldItsRecoveryCaused` reaches `billingpass.go:297-301` through a lost create, an attached subscription and a listing recovery in EUR. | **No.** Without the filter the pass returns `errBillingUnderReview`. | |
| +| **Opus claims 1–4** | Corrected (`billingservice.go:790-796`, `billingretirement.go:970-974`, the dispositions' "Where"). The `ENABLEMENT.md` change is on the launch branch, which I did not read. | n/a | |
| + |
| +**The gate failure on `2618403f`.** `TestABoundSessionReadBackAsNobodysIsReviewed` was right before round twenty-two's P3-2 and wrong after it. |
| +- That fix records `sub_paid`, which names the intent under the other account's name, against the intent. The first hold is therefore the attachment. |
| +- The test now asserts both holds: the attachment, then the unproven expiry after the obligation is deleted by hand. It is right now. |
| +- The hand deletion makes a state that no code path produces, but it isolates the second hold honestly. |
| +- It would fail without P3-2, because the first `heldRetire` wants `"sub_paid"`. |
| + |
| +**Severity, from the code at `2618403f`.** Astra was right. |
| +- The handler went reserve → `tombstoneAccount` → `DeleteAccount`, and only the last of these re-read the reviews. |
| +- A dispute committed in between therefore left the manifest retired and the account live, under a reservation that blocks checkout and a dispute stage one cannot resolve. That is a P2. |
| +- My round-twenty-three acceptance missed it. |
| + |
| +## NNNN: does the point of no return answer it? |
| + |
| +I enumerated the orderings, given SQLite's one writer. A deferred transaction that read before another writer committed fails its write upgrade with BUSY rather than going on. |
| + |
| +| Ordering | Result | |
| +|---|---| |
| +| Evidence, then reserve | Reserve refuses. | |
| +| Reserve, then evidence, then commit | The evidence withdraws the reservation. The commit finds nothing reserved and refuses. The handler releases; nothing outside was touched. ✓ | |
| +| Reserve, then commit, then evidence, then tombstone, then delete | The evidence lands on an account whose retirement is committed. `DeleteAccount` passes it (`committed=true`) and the review is listed with `account_deleted`. ✓ | |
| +| Evidence written by an older binary (no withdrawal) before the commit | The commit re-reads the reviews and refuses (case 2). ✓ | |
| +| Death between the commit and the tombstone | The committed reservation stays. Checkout stays blocked and later evidence lands visibly on a live account. A retry finishes, because reserve with `committed=true` skips the review clause. ✓ | |
| +| Another condition after the commit (intent, month, active subscription) | Blocked by the reservation: `beginBillingCheckout` at `:363`, `freezeBillingMonth` at `:1400`. A first purchase needs a standing intent. ✓ | |
| +| An **older binary** deleting during a roll | It has no commit step, so Astra's race is reproduced by that binary. Its `ReapAccount` (at `2618403f`) also refuses an open review, leaving the account "tombstoned but live" on that console until it is upgraded. This is covered only by the roll rule; state it there. | |
| + |
| +**Is evidence after the commit handled honestly?** Yes, with one correction. After the commit no intent stands, and none can be created. Every review that can open is therefore a `checkout_fate_disputed`: |
| +- a subscription naming a consumed or retired intent goes through `classifySubscriptionTx`; |
| +- a late session refusal goes through the consumed-intent branch at `:1256-1272`. |
| + |
| +Such a review is listed and the door finds it, but it answers 409 "held", exactly as on a live account. The test itself asserts that. So it is "listed and held with no disposition", not "resolvable" (claim 1). |
| + |
| +**`ReapAccount` passing `true` unconditionally** (`store.go:1370`) is the right call. The manifest is authoritative, and refusing the reap would leave a retired account's memberships live here. The review stays visible, and the reap still refuses an active subscription, an intent or an open month. |
| + |
| +## Findings |
| + |
| +### P3-1. The production commit call is untested |
| +- **Where:** `ownership.go:160-170`. |
| +- **Scenario:** delete the `commitBillingRetirement` call from the handler. Every test still passes: |
| + - the new test drives `reserveBillingRetirement` and `commitBillingRetirement` directly; |
| + - the handler tests in `deletion_test.go` and `controlplane_test.go` have no review opening between reserve and tombstone. |
| +- **Effect:** the handler would then publish the tombstone and refuse in `DeleteAccount`. That is Astra's P2 restored with green tests. |
| +- **Fix:** add a handler test in which a hook in `tombstoneAccount` (or the release store) files a dispute through the production classification. Assert either that the deletion finishes with the review listed, or, with the dispute filed before the commit, that nothing is written to the bucket. |
| + |
| +### P3-2. A failed commit releases a committed reservation that is not its own |
| +- **Where:** `ownership.go:161-163`, together with `releaseBillingRetirement` (`billingstore.go:1640`), which deletes regardless of `committed_at`. |
| +- **Scenario:** |
| + 1. Two deletion requests for one account arrive from two tabs, or two consoles on one database. |
| + 2. Request A commits and is writing the tombstone. |
| + 3. Request B's `commitBillingRetirement` fails on a transient error (BUSY, `Begin`) and calls release, which deletes A's committed row. |
| + 4. A dispute then opens. |
| + 5. A's `DeleteAccount` sees `committed=false` and refuses after the tombstone. That is the P2 state again, and checkout is also unbarred in the meantime. |
| +- **Fix:** release only on `errBillingRetirement`, or make that branch `DELETE … AND committed_at=0`. |
| + |
| +### P3-3. Reservations from before the column read as pending |
| +- **Where:** `billingretirement.go:176` (the default is 0). |
| +- **Scenario:** |
| + 1. An older binary's tombstone failed ambiguously and the reservation was kept, as intended. |
| + 2. After the upgrade, a dispute withdraws that reservation as pending. |
| + 3. The account can now open a checkout or freeze a month although its manifest may be tombstoned. |
| + 4. The later reap refuses on the new intent or month, and the account is tombstoned but live. |
| +- **Fix:** when the column is added, set `committed_at=started_at` on existing rows (only in the migration that adds it), or state it in the roll rule. |
| + |
| +### P3-4. A doc comment is detached |
| +- **Where:** `billingstore.go:1605-1613`. `accountEvenIfDeleted` was inserted between `releaseBillingRetirement`'s comment and its function. |
| +- **Effect:** godoc gives the new lookup the release's comment, and the release has none. The release comment's "called only when … no release store is configured" also omits the new commit-failure caller. |
| +- **Fix:** move the lookups below the release and add the commit-failure case to its comment. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **"Listed and resolvable under the deleted account"** appears in the design's "Stage one as built", the dispositions (Astra 1 and Opus P3-2), `checkBillingRetirement`'s comment (`billingstore.go:1505-1513`), `ReapAccount`'s comment (`store.go:1366-1369`) and the dispositions' "Stated and not changed". Every review that can open after the commit is a dispute, and the door answers 409 to every dispute. It should read "listed, and held with no stage-one disposition". |
| +2. **"SQLite serialises the two"** (dispositions) is true only of binaries that commit. An older binary has no commit step and reproduces the race. The roll rule should name it. |
| + |
| +## OOOO: anything required before acceptance? |
| + |
| +No code is required. P3-1 is the one I would most want before merge, because without it the fix for the round's only P2 is unguarded at its production site. The rest, and the two claims, are fixes to make, not gates. What remains: |
| +- decision 15, with the predicate corrected last round (every invoice void or draft, no succeeded or processing charge, `incomplete_expired`); |
| +- the test-mode rehearsal and its first-check gate; |
| +- the roll rule covering older binaries' deletion and reap; |
| +- the owner's approvals. |
| + |
| +## Verify in Stripe test mode before enabling |
| + |
| +Last round's six items are unchanged: |
| +1. Whether Checkout creates a Subscription before the session completes: with 3-D Secure abandoned, a declined card then another, and a delayed debit. |
| +2. What an `incomplete_expired` subscription leaves: its invoices, charges and PaymentIntents. |
| +3. SEPA or Bacs through a failed debit. |
| +4. The order of `customer.subscription.created` against `checkout.session.completed`. |
| +5. Whether a session's or a subscription's metadata can be edited after creation, and by whom. |
| +6. What can change on a canceled subscription. |
| + |
| +Plus Astra's ten-item list: the catalog identities, the GBP one-form purchase, expiry racing payment, idempotency, lost-create recovery, GBP overages and quantity, legacy EUR reuse. |
| + |
| +Nothing in this delta adds a provider dependency. The per-element listing decode rests on the response shape, which item 7 of that list (lost-create recovery) already exercises. |
| + |
| +**Verdict: accept with fixes** (P3-1 to P3-4 and the two claims; decision 15, the rehearsal gate and the roll rule before enabling). |